fix test race condition; remove verbose global #457
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mfridman
commented
Jan 28, 2023
| @@ -11,6 +11,20 @@ import ( | |||
| "sync" | |||
| ) | |||
|
|
|||
| type Direction string | |||
There was a problem hiding this comment.
The ParseSQLMigration took 2 booleans with a signature like (r io.Reader, direction, verbose bool).
It's waaaay too easy for the caller to mix up the booleans, so instead, the second param is now a well-defined type to avoid any footguns.
mfridman
commented
Jan 28, 2023
| @@ -23,15 +37,37 @@ const ( | |||
| gooseStatementEndDown // 6 | |||
| ) | |||
|
|
|||
| type stateMachine parserState | |||
| type stateMachine struct { | |||
There was a problem hiding this comment.
I think it makes sense to attach the verbose bool to the struct and hang a print helper.
We pass around the stateMachine struct as a pointer.
mfridman
commented
Jan 28, 2023
| @@ -73,6 +74,11 @@ func TestMain(m *testing.M) { | |||
| migrationsDir = filepath.Join("testdata", *dialect, "migrations") | |||
| seedDir = filepath.Join("testdata", *dialect, "seed") | |||
|
|
|||
| if err := goose.SetDialect(*dialect); err != nil { | |||
There was a problem hiding this comment.
The dialect is set once within the e2e tests, we can do this here in TestMain.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed a race condition in the tests, having to do with setting verbose globally in
sqlparser.https://github.com/pressly/goose/actions/runs/4032205783/jobs/6931938495
This PR updates the internal/sqlparser. Changes the
stateMachineto a well-defined struct which we pass around as a pointer. The caller sets the direction (well-defined) and the verbose bool, no more global state in sqlparser.For convenience, one can set env
DEBUG_TEST=truein tests to see those verbose parsing statements.DEBUG_TEST=true go test -race -count=1 -v -timeout=10m ./internal/sqlparser/...