Skip to content

Fix tests with Go1.17 #1066

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
QuLogic wants to merge 1 commit into from
Closed

Fix tests with Go1.17 #1066

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions cmd/submit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ func TestSubmitFiles(t *testing.T) {
if assert.NoError(t, err) {
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"])
Copy link
Copy Markdown
Member

@iHiD iHiD Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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? :)

Copy link
Copy Markdown
Contributor

@nywilken nywilken Sep 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

@ErikSchierboom ErikSchierboom Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@nywilken nywilken Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert.Equal(t, "This is the readme.", submittedFiles["README.md"])
assert.Regexp(t, "submitted successfully", Err)
}
Expand Down Expand Up @@ -468,7 +468,7 @@ func TestSubmitFilesForTeamExercise(t *testing.T) {
assert.Equal(t, 2, 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"])
}

func TestSubmitOnlyEmptyFile(t *testing.T) {
Expand Down Expand Up @@ -561,7 +561,7 @@ func fakeSubmitServer(t *testing.T, submittedFiles map[string]string) *httptest.
if err != nil {
t.Fatal(err)
}
submittedFiles[fileHeader.Filename] = string(body)
submittedFiles[filepath.Base(fileHeader.Filename)] = string(body)
Copy link
Copy Markdown
Contributor

@nywilken nywilken Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@iHiD iHiD Sep 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

@nywilken nywilken Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@kytrinyx kytrinyx Sep 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor

@nywilken nywilken Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

}

fmt.Fprint(w, "{}")
Expand Down