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

ddl: make TestModifyColumnNullToNotNull test stable #13223

Merged
merged 3 commits into from
Nov 7, 2019

Conversation

bb7133
Copy link
Member

@bb7133 bb7133 commented Nov 6, 2019

What problem does this PR solve?

This PR tries to fix a potentially failed assertion that makes TiDB unit test keep hanging and failed because of timeout.

What is changed and how it works?

The code:

tk2.MustExec("insert into t1 values ();")

in TestModifyColumnNullToNotNull has following flaws:

  1. Made a write during the schema change of t1, in which leads a potential information schema is changed error.
  2. When the assertion MustExec failed, only ddl worker goroutine exits, which leaves ddl job never ends.

This PR removes MustExec and does the make outside of hook, while changing the time of hook called to avoid information schema is changed error.

PS: There are still some other codes that use assertion inside of hook in ddl tests, we should refine them all to avoid similar timeout failures, I plan to do the refinements later.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • NA

Side effects

  • NA

Related changes

  • NA

Release note

  • NA

@sre-bot
Copy link
Contributor

sre-bot commented Nov 6, 2019

Thanks for your PR.
This PR will be closed by bot because you already had 3 opened PRs, close or merge them before submitting a new one.
#9431 , #12056 , #13033

@bb7133
Copy link
Member Author

bb7133 commented Nov 6, 2019

PTAL @crazycs520 @tiancaiamao @zimulala

@codecov
Copy link

codecov bot commented Nov 6, 2019

Codecov Report

Merging #13223 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #13223   +/-   ##
===========================================
  Coverage   80.1406%   80.1406%           
===========================================
  Files           469        469           
  Lines        111323     111323           
===========================================
  Hits          89215      89215           
  Misses        15217      15217           
  Partials       6891       6891

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 6, 2019
ddl/db_test.go Outdated
@@ -2725,18 +2723,18 @@ func (s *testDBSuite1) TestModifyColumnNullToNotNull(c *C) {
}
c2 := getModifyColumn()
if mysql.HasPreventNullInsertFlag(c2.Flag) {
Copy link
Contributor

@Deardrops Deardrops Nov 7, 2019

Choose a reason for hiding this comment

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

Do we need this check? I think c2.Flag always HasPreventNullInsertFlag in this place.

Copy link
Member Author

@bb7133 bb7133 Nov 7, 2019

Choose a reason for hiding this comment

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

You're right, it's not necessary.

@bb7133
Copy link
Member Author

bb7133 commented Nov 7, 2019

/run-all-tests

Copy link
Contributor

@Deardrops Deardrops left a comment

Choose a reason for hiding this comment

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

LGTM

@Deardrops Deardrops added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 7, 2019
@bb7133 bb7133 added the status/can-merge Indicates a PR has been approved by a committer. label Nov 7, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Nov 7, 2019

/run-all-tests

Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/test status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants