Skip to content

Fix running of auth tests#1227

Closed
dveeden wants to merge 1 commit intopingcap:masterfrom
dveeden:auth_tests
Closed

Fix running of auth tests#1227
dveeden wants to merge 1 commit intopingcap:masterfrom
dveeden:auth_tests

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