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

Better error message checking #1249

Merged
merged 2 commits into from
Nov 25, 2019

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Nov 18, 2019

Currently, we check exactly matching of error message return by go
standard library. It's working nowaday but not guaranteed to work in
upcoming go version. At least in go1.14, all the certificate expried
message checking test will fail, due to the change of error message
format in net/http.

To fix this, we only check that error message contains the message we
want, e.g certificate expired error should contain word "expired".

While at it, also add some require.NoError to make sure no editor
complains above "can lead to nil pointer dereference".

@cuonglm cuonglm requested review from mstoykov, imiric and na-- November 18, 2019 04:07
@cuonglm cuonglm added this to the v0.27.0 milestone Nov 18, 2019
Currently, we check exactly matching of error message return by go
standard library. It's working nowaday but not guaranteed to work in
upcoming go version. At least in go1.14, all the certificate expried
message checking test will fail, due to the change of error message
format in net/http.

To fix this, we only check that error message contains the message we
want, e.g certificate expired error should contain word "expired".

While at it, also add some require.NoError to make sure no editor
complains above "can lead to nil pointer dereference".
@cuonglm cuonglm force-pushed the cuonglm/refactoring-error-message-check-in-test branch from 9306558 to 7c9ded2 Compare November 18, 2019 04:16
@codecov-io
Copy link

codecov-io commented Nov 18, 2019

Codecov Report

Merging #1249 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1249      +/-   ##
=========================================
+ Coverage   75.28%   75.3%   +0.01%     
=========================================
  Files         149     149              
  Lines       10841   10841              
=========================================
+ Hits         8162    8164       +2     
+ Misses       2212    2210       -2     
  Partials      467     467
Impacted Files Coverage Δ
stats/cloud/collector.go 77.6% <0%> (+0.61%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5f6881...3410b55. Read the comment docs.

mstoykov
mstoykov previously approved these changes Nov 18, 2019
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

LGTM, but can you link to some issues/CL around this as we might need to do other changes ?

js/runner_test.go Outdated Show resolved Hide resolved
@cuonglm
Copy link
Contributor Author

cuonglm commented Nov 18, 2019

LGTM, but can you link to some issues/CL around this as we might need to do other changes ?

You mean this golang/go#34066?

@na-- na-- merged commit e3aa1aa into master Nov 25, 2019
@na-- na-- deleted the cuonglm/refactoring-error-message-check-in-test branch November 25, 2019 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants