Skip to content
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.

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

Update unit tests to support Go 1.22 http.ServerMux changes #3412

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DiegoDev2
Copy link
Contributor

very good, the pr is not ready yet, but I would like you to see the small changes I made, there is a particular test that gives me problems but I am working on solving it. @gmlewis

This PR is directed to the problem #3409

Copy link
Contributor Author

@DiegoDev2 DiegoDev2 left a comment

Choose a reason for hiding this comment

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

It should be noted that I uploaded the file where I was working by mistake. What is repos_contents_test.go

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 1, 2025

Note that the PR that solves #3409 (hopefully this one) will also revert the changes to:

  • README.md
  • script/test.sh

made here: https://github.com/google/go-github/pull/3410/files
(All other go.mod changes in #3410 will be left alone, obviously.)

@gmlewis gmlewis changed the title tests-remove-GODEBUG-httpmuxgo121 Update unit tests to support Go 1.22 http.ServerMux changes Jan 1, 2025
@@ -916,6 +916,9 @@ func TestRepositoriesService_ListBranches(t *testing.T) {
})
}

// Test removed: "feat/branch-50%" registered with the same pattern as "b".
// Reason: The new logic in ServeMux requires unique patterns.
// This test is no longer necessary as the pattern is already covered by another test.
Copy link
Collaborator

Choose a reason for hiding this comment

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

As you proceed, note that the feat/branch-50% case is important because of:
#2948
So if it is the same pattern as "b", then the "b" case should be removed and the more challenging case should remain.

Although I'm a bit confused by the comment because it says "Test removed" but I'm not seeing its removal (which is actually good... so maybe the comment just needs updating?).

@DiegoDev2
Copy link
Contributor Author

Note that the PR that solves #3409 (hopefully this one) will also revert the changes to:

* README.md

* script/test.sh

made here: https://github.com/google/go-github/pull/3410/files (All other go.mod changes in #3410 will be left alone, obviously.)

Could you clarify why we should revert the changes in README.md and script/test.sh from PR #3410? Understanding the reasoning will help ensure this PR aligns with the project's goals.

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 1, 2025

Could you clarify why we should revert the changes in README.md and script/test.sh from PR #3410? Understanding the reasoning will help ensure this PR aligns with the project's goals.

From the description of #3409:

It would be greatly appreciated if an experienced Go developer would update this repo's
legacy unit tests so that go test ./... passes without the use of setting the GODEBUG
environment variable.

Reverting the changes to README.md and script/test.sh accomplish this goal.

@DiegoDev2
Copy link
Contributor Author

Could you clarify why we should revert the changes in README.md and script/test.sh from PR #3410? Understanding the reasoning will help ensure this PR aligns with the project's goals.

From the description of #3409:

It would be greatly appreciated if an experienced Go developer would update this repo's

legacy unit tests so that go test ./... passes without the use of setting the GODEBUG

environment variable.

Reverting the changes to README.md and script/test.sh accomplish this goal.

I'm not understanding, I leave the tests as before and revert the changes to #3409?

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 1, 2025

I'm not understanding, I leave the tests as before and revert the changes to #3409?

No.

You remove these two lines from script/test.sh:

# TODO(gmlewis): Remove this when #3409 is resolved.
export GODEBUG=httpmuxgo121=1

and you make sure all unit tests pass without these two lines using Go 1.23.4.

Then you remove lines 18-29 from README.md. That's it.

@DiegoDev2
Copy link
Contributor Author

I'm not understanding, I leave the tests as before and revert the changes to #3409?

No.

You remove these two lines from script/test.sh:

# TODO(gmlewis): Remove this when #3409 is resolved.

export GODEBUG=httpmuxgo121=1

and you make sure all unit tests pass without these two lines using Go 1.23.4.

Then you remove lines 18-29 from README.md. That's it.

I understand, it's okay. Thank you for your explanation

@DiegoDev2
Copy link
Contributor Author

Hi, sorry for the delay, while I'm solving errors, errors continue to appear xD

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 8, 2025

Hi, sorry for the delay, while I'm solving errors, errors continue to appear xD

No worries, @DiegoDev2 - there's no rush.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants