Fix tests with Go1.17#1066
Conversation
`Part.FileName` now applies `filepath.Base` to the return value [1]. For compatibility with older versions, this now explicitly applies `Base` when fetching `FileName` always. [1] https://go.dev/doc/go1.17#mime/multipart
| assert.Equal(t, 3, len(submittedFiles)) | ||
| assert.Equal(t, "This is file 1.", submittedFiles["file-1.txt"]) | ||
| assert.Equal(t, "This is file 2.", submittedFiles["subdir/file-2.txt"]) | ||
| assert.Equal(t, "This is file 2.", submittedFiles["file-2.txt"]) |
There was a problem hiding this comment.
Hey! I'm a bit unclear on this test changing. If someone submits:
file-1.txt
subdir/file-2.txt
file-2.txt
what happens? Have we lost that ability, or just lost a test for that? Or something else? :)
There was a problem hiding this comment.
This change is just to fix the response in the test case, as the multipart form used in the test server now returns the filename without the directory information link to Go source
Using Go 1.20.8
This is the fileheader information that gets returned in the test. You can see the form-data is correct, which will be handled by the upstream API as is. But the returned Filename contains just the base filename, which is what is causing the test to fail.
=== RUN TestSubmitFiles
{
"Filename": "file-1.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"file-1.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "file-2.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "README.md",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"README.md\""
],
"Content-Type": [
"application/octet-stream"
]
}
--- FAIL: TestSubmitFiles (0.00s)With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.
=== RUN TestSubmitFiles
{
"Filename": "file-1.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"file-1.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "file-2.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "README.md",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"README.md\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 19
}
-- PASS: TestSubmitFiles (0.00s)
There was a problem hiding this comment.
With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.
I see, but we are losing information now, as we're no longer testing the subdir, which is really important. Can the test be restructured to allow testing the filename and the directory?
There was a problem hiding this comment.
Ah that's a fair point. I was thinking solely from the point of the submitted file content and not the directory tree. We can pull the unmodified filename from the Header to validate instead of using fileHeader.Filename. Which looking at the Go PR where this change was introduced implies as the thing to do. I opened a PR with a fix that keeps the directory info intact. Refer to #1115
| t.Fatal(err) | ||
| } | ||
| submittedFiles[fileHeader.Filename] = string(body) | ||
| submittedFiles[filepath.Base(fileHeader.Filename)] = string(body) |
There was a problem hiding this comment.
So this particular change can be dropped once the Go version for running the test on CI is bumped from 1.15 to 1.17+, as fileheader.Filename will already have the directory information stripped from it.
There was a problem hiding this comment.
So this particular change can be dropped once the Go version for running the test on CI is bumped from 1.15 to 1.17+, as fileheader.Filename will already have the directory information stripped from it.
Thanks :) Shall we do this as part of this PR?
There was a problem hiding this comment.
Given this is a small change and ready to go. I would suggest merging as is and creating a new PR for bumping Go versions.
That said, since the go.mod is still at 1.15 we may not want to remove this if the expectation is to have Go 1.15 as the target toolchain version.
——
My rationale for a separate PR is so that we can look into adding a matrix to test against different Go versions during CI. This would be good to validate that new changes don’t break compatibly with older supported go versions. For example the CLI releases are made with 1.19 but the CI is 1.15. A matrix can help validate each version.
As alternative maybe @ErikSchierboom and @kytrinyx would be up for moving to a higher version of Go for dev and bumping the go.mod file version. Which also involves updating the contributing guide to call out the min dev version.
These are straight forward changes that won’t introduce any breaking changes but will have a little meat to them. So a separate PR feels right.
There was a problem hiding this comment.
I agree that it makes sense to do it in a separate PR (and looking into a matrix seems sensible).
I don't have any objections to bumping the Go version (but also I don't actually do any real work on this anymore, so I don't have skin in the game).
There was a problem hiding this comment.
@iHiD just a heads up I'm working on a PR to bump Go versions. I will work on getting it opened this week. It currently sits as a draft at nywilken#1
Do you prefer I open a discussion on the Exercism forum before opening a PR?
| assert.Equal(t, 3, len(submittedFiles)) | ||
| assert.Equal(t, "This is file 1.", submittedFiles["file-1.txt"]) | ||
| assert.Equal(t, "This is file 2.", submittedFiles["subdir/file-2.txt"]) | ||
| assert.Equal(t, "This is file 2.", submittedFiles["file-2.txt"]) |
There was a problem hiding this comment.
This change is just to fix the response in the test case, as the multipart form used in the test server now returns the filename without the directory information link to Go source
Using Go 1.20.8
This is the fileheader information that gets returned in the test. You can see the form-data is correct, which will be handled by the upstream API as is. But the returned Filename contains just the base filename, which is what is causing the test to fail.
=== RUN TestSubmitFiles
{
"Filename": "file-1.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"file-1.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "file-2.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "README.md",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"README.md\""
],
"Content-Type": [
"application/octet-stream"
]
}
--- FAIL: TestSubmitFiles (0.00s)With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.
=== RUN TestSubmitFiles
{
"Filename": "file-1.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"file-1.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "file-2.txt",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"subdir/file-2.txt\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 15
},
{
"Filename": "README.md",
"Header": {
"Content-Disposition": [
"form-data; name=\"files[]\"; filename=\"README.md\""
],
"Content-Type": [
"application/octet-stream"
]
},
"Size": 19
}
-- PASS: TestSubmitFiles (0.00s)
|
OK, cool. Thank you for the reviews and feedback! Let's get this merged then. @ErikSchierboom Could you lead on this pls? |
| assert.Equal(t, 3, len(submittedFiles)) | ||
| assert.Equal(t, "This is file 1.", submittedFiles["file-1.txt"]) | ||
| assert.Equal(t, "This is file 2.", submittedFiles["subdir/file-2.txt"]) | ||
| assert.Equal(t, "This is file 2.", submittedFiles["file-2.txt"]) |
There was a problem hiding this comment.
With the fix in place you see the fileheader information is still the same but the test is now just expecting the base filename so all is well.
I see, but we are losing information now, as we're no longer testing the subdir, which is really important. Can the test be restructured to allow testing the filename and the directory?
Following RFC 7578, Go 1.17+ strips the directory information in fileHeader.Filename. This change updates the test server to use Header["Content-Disposition"] for obtaining the unmodified filename for validating the submitted files directory tree in the request. This work builds on the PR opened by @QuLogic exercism#1066
) Following RFC 7578, Go 1.17+ strips the directory information in fileHeader.Filename. This change updates the test server to use Header["Content-Disposition"] for obtaining the unmodified filename for validating the submitted files directory tree in the request. This work builds on the PR opened by @QuLogic #1066 Co-authored-by: Erik Schierboom <erik_schierboom@hotmail.com>
|
Closed by #1115 |
5 participants
Part.FileNamenow appliesfilepath.Baseto the return value [1]. For compatibility with older versions, this now explicitly appliesBasewhen fetchingFileNamealways.Note that CI is on 1.15 which is no longer supported; I can update that to supported versions if needed.
[1] https://go.dev/doc/go1.17#mime/multipart