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

net/url: fail TestParseErrors test when getting an unwanted error #33876

Closed

Conversation

stefanb
Copy link
Contributor

@stefanb stefanb commented Aug 27, 2019

The TestParseErrors test function was not strict with unwanted errors
received from url.Parse(). It was not failing in such cases, now it does

Fixes #33646
Updates #29098

The TestParseErrors test function was not strict with unwanted errors
received from url.Parse(). It was not failing in such cases, now it does.

Updates golang#33646 and golang#29098
@googlebot googlebot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Aug 27, 2019
@gopherbot
Copy link
Contributor

This PR (HEAD: de1e898) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/191966 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@av86743
Copy link

av86743 commented Aug 27, 2019

@gopherbot
Copy link
Contributor

Message from Štefan Baebler:

Patch Set 1:

This fixes the tests for #33646, which was introduced by #29098

Originally committed as #32954


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Filippo Valsorda:

Patch Set 1: Run-TryBot+1 Code-Review+2

(1 comment)

Thank you! Minor note on the commit message.


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=b6a0fa38


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1:

Build is still in progress...
This change failed on freebsd-amd64-12_0:
See https://storage.googleapis.com/go-build-log/b6a0fa38/freebsd-amd64-12_0_6317f03b.log

Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 1: TryBot-Result-1

10 of 21 TryBots failed:
Failed on freebsd-amd64-12_0: https://storage.googleapis.com/go-build-log/b6a0fa38/freebsd-amd64-12_0_6317f03b.log
Failed on linux-amd64: https://storage.googleapis.com/go-build-log/b6a0fa38/linux-amd64_6cc6e1f5.log
Failed on openbsd-amd64-64: https://storage.googleapis.com/go-build-log/b6a0fa38/openbsd-amd64-64_70ac2673.log
Failed on linux-386: https://storage.googleapis.com/go-build-log/b6a0fa38/linux-386_0fcc1a21.log
Failed on windows-amd64-2016: https://storage.googleapis.com/go-build-log/b6a0fa38/windows-amd64-2016_f49a2c5f.log
Failed on js-wasm: https://storage.googleapis.com/go-build-log/b6a0fa38/js-wasm_5d31b321.log
Failed on android-amd64-emu: https://storage.googleapis.com/go-build-log/b6a0fa38/android-amd64-emu_2c8c7e5f.log
Failed on nacl-amd64p32: https://storage.googleapis.com/go-build-log/b6a0fa38/nacl-amd64p32_6a6db042.log
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/b6a0fa38/windows-386-2008_34991e50.log
Failed on linux-amd64-race: https://storage.googleapis.com/go-build-log/b6a0fa38/linux-amd64-race_9b2bfe1b.log

Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed.


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@av86743
Copy link

av86743 commented Aug 27, 2019

Surprise, surprise.

@gopherbot
Copy link
Contributor

Message from Štefan Baebler:

Patch Set 1:

(1 comment)

Patch Set 1: Run-TryBot+1 Code-Review+2

(1 comment)

Thank you! Minor note on the commit message.


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@stefanb
Copy link
Contributor Author

stefanb commented Aug 27, 2019

@av86743 no surprise there, this change fixes the TEST to detect the bug #33646 and rightfully fail until the #33646 is fixed.

@av86743
Copy link

av86743 commented Aug 27, 2019

... no surprise there, this change fixes the TEST to detect the bug #33646 and rightfully fail until the #33646 is fixed.

@stefanb Your change is identical to https://go-review.googlesource.com/c/go/+/185080/ - why did you create an exact replica of what has been reverted just before (because it fails)? Instead of uploading something that works.

Is it funnier this way?

@stefanb
Copy link
Contributor Author

stefanb commented Aug 27, 2019

@av86743 Because this needs to get in as soon as the #33646 is fixed. In the same
https://go-review.googlesource.com/c/go/+/185080 I was kindly asked by maintainers to re-submit it for a later merge.

@gopherbot
Copy link
Contributor

Message from Filippo Valsorda:

Patch Set 1:

(2 comments)

Please fix the failing case, which is now expected to return an error, per #33646.


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@av86743
Copy link

av86743 commented Aug 27, 2019

So, basically, you don't care that .Errorf() message contains "...; want no error".

@gopherbot
Copy link
Contributor

This PR (HEAD: 53beeb5) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/191966 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: bd26c57) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/191966 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

This PR (HEAD: e6844c5) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/191966 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link
Contributor

Message from Filippo Valsorda:

Patch Set 5: Run-TryBot+1


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 5:

TryBots beginning. Status page: https://farmer.golang.org/try?commit=a73c20fb


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

Message from Štefan Baebler:

Patch Set 5:

Patch Set 1:

(2 comments)

Please fix the failing case, which is now expected to return an error, per #33646.

Tnx for the pointer!
Both mysql test cases fixed - adjusted expectation to get an error from parser.


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@stefanb
Copy link
Contributor Author

stefanb commented Aug 27, 2019

So, basically, you don't care that .Errorf() message contains "...; want no error".

@av86743 That is a testing construct to fail a test if the tested Parse function returned an error that was not wanted.

gopherbot pushed a commit that referenced this pull request Aug 27, 2019
The TestParseErrors test function was not strict with unwanted errors
received from url.Parse(). It was not failing in such cases, now it does

Fixes #33646
Updates #29098

Change-Id: I069521093e2bff8b1fcd41ffd3f9799f3108bc61
GitHub-Last-Rev: e6844c5
GitHub-Pull-Request: #33876
Reviewed-on: https://go-review.googlesource.com/c/go/+/191966
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
@gopherbot
Copy link
Contributor

Message from Gobot Gobot:

Patch Set 5: TryBot-Result+1

TryBots are happy.


Please don’t reply on this GitHub thread. Visit golang.org/cl/191966.
After addressing review feedback, remember to publish your drafts!

@gopherbot
Copy link
Contributor

This PR is being closed because golang.org/cl/191966 has been merged.

@gopherbot gopherbot closed this Aug 27, 2019
@stefanb stefanb deleted the net-url-testparseerrors-fix-again branch August 28, 2019 07:25
tomocy pushed a commit to tomocy/go that referenced this pull request Sep 1, 2019
The TestParseErrors test function was not strict with unwanted errors
received from url.Parse(). It was not failing in such cases, now it does

Fixes golang#33646
Updates golang#29098

Change-Id: I069521093e2bff8b1fcd41ffd3f9799f3108bc61
GitHub-Last-Rev: e6844c5
GitHub-Pull-Request: golang#33876
Reviewed-on: https://go-review.googlesource.com/c/go/+/191966
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
t4n6a1ka pushed a commit to t4n6a1ka/go that referenced this pull request Sep 5, 2019
The TestParseErrors test function was not strict with unwanted errors
received from url.Parse(). It was not failing in such cases, now it does

Fixes golang#33646
Updates golang#29098

Change-Id: I069521093e2bff8b1fcd41ffd3f9799f3108bc61
GitHub-Last-Rev: e6844c5
GitHub-Pull-Request: golang#33876
Reviewed-on: https://go-review.googlesource.com/c/go/+/191966
Run-TryBot: Filippo Valsorda <[email protected]>
TryBot-Result: Gobot Gobot <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

net/url: mysql://signer@tcp(mysql:3306)/notarysigner no longer parses after 1.12.8
4 participants