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

Fix running of auth tests #1227

Closed
wants to merge 1 commit into from
Closed

Conversation

dveeden
Copy link
Contributor

@dveeden dveeden commented May 18, 2021

What problem does this PR solve?

The tests in auth/auth_test.go were not run.

To see the issue modify auth/auth_test.go to make a test case that should fail. Then run make test. The expected outcome is a failed test run, but in fact it doesn't show any errors.

What is changed and how it works?

  • This adds check.TestingT() to auth/auth_test.go
  • This adds a simple check in test.sh to test for similar cases.

Check List

Tests

  • Unit test

@CLAassistant
Copy link

CLAassistant commented May 18, 2021

CLA assistant check
All committers have signed the CLA.

@dveeden
Copy link
Contributor Author

dveeden commented May 18, 2021

[dvaneeden@dve-carbon parser]$ make test
GO111MODULE=on go build -o bin/goyacc goyacc/main.go goyacc/format_yacc.go
gofmt (simplify)
bin/goyacc -o parser.go -p yy -t Parser parser.y
Parse table entries: 2277465 of 5314635, x 16 bits == 4554930 bytes
bin/goyacc -o hintparser.go -p yyhint -t hintParser hintparser.y
Parse table entries: 16703 of 28560, x 16 bits == 33406 bytes
sh test.sh
ok  	github.com/pingcap/parser	3.498s	coverage: 75.7% of statements in ./...
ok  	github.com/pingcap/parser/ast	0.884s	coverage: 41.3% of statements in ./...
ok  	github.com/pingcap/parser/auth	0.121s	coverage: 0.8% of statements in ./... [no tests to run]
ok  	github.com/pingcap/parser/charset	0.142s	coverage: 1.0% of statements in ./...
ok  	github.com/pingcap/parser/format	0.132s	coverage: 1.3% of statements in ./...
?   	github.com/pingcap/parser/goyacc	[no test files]
ok  	github.com/pingcap/parser/model	0.142s	coverage: 2.0% of statements in ./...

This is the output without this PR. Note the [no tests to run].

@dveeden
Copy link
Contributor Author

dveeden commented May 18, 2021

Looks like #1220 has a similar fix, but doesn't change test.sh.

@dveeden
Copy link
Contributor Author

dveeden commented May 18, 2021

/run-all-tests

@dveeden dveeden requested a review from tiancaiamao May 21, 2021 11:50
@dveeden dveeden mentioned this pull request May 24, 2021
@dveeden
Copy link
Contributor Author

dveeden commented May 24, 2021

Changed test.sh based on https://github.com/koalaman/shellcheck

@dveeden dveeden requested a review from kennytm May 26, 2021 05:57
@@ -1 +1,14 @@
#!/bin/sh

# If 'check.TestingT' is not used in any of the *_test.go files in a subdir no tests will run.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Let's run this check in the make test of Makefile.

@tiancaiamao
Copy link
Collaborator

This is included in #1232 already?

@dveeden
Copy link
Contributor Author

dveeden commented Jun 1, 2021

This is included in #1232 already?

Both #1232 and #1236 are based on top of this commit/PR.

Note that #1220 has a similar fix but doesn't include the changes in test.sh

@tiancaiamao
Copy link
Collaborator

This is included in #1232 already?

Both #1232 and #1236 are based on top of this commit/PR.

Note that #1220 has a similar fix but doesn't include the changes in test.sh

We should have merge this one first ...
Now that #1232 is merged already, I'll close this one.

@tiancaiamao tiancaiamao closed this Jun 2, 2021
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