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

introduce runTestsParallel #1489

Closed
wants to merge 14 commits into from
Closed

Conversation

shogo82148
Copy link
Contributor

Description

Please explain the changes you made here.

Checklist

  • Code compiles correctly
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary
  • Added myself / the copyright holder to the AUTHORS file

@coveralls
Copy link

coveralls commented Oct 7, 2023

Coverage Status

coverage: 82.713%. remained the same when pulling 008e97a on introduce-run-tests-parallel into 3798012 on master.

t.Run("default", func(t *testing.T) {
dbt := &DBTest{t, db}
t.Cleanup(func() {
dbt.db.Exec("DROP TABLE IF EXISTS test")
})
test(dbt)
dbt.db.Exec("DROP TABLE IF EXISTS test")
Copy link
Contributor

@dolmen dolmen Oct 18, 2023

Choose a reason for hiding this comment

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

This is now redundant with the Cleanup.

if _, err := rand.Read(buf[:]); err != nil {
t.Fatal(err)
}
tableName := fmt.Sprintf("test_%x", buf[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a non-zero risk of having two tests started with the same table name.

It would be better to use the index of the iteration of the for loop and a hash of the test name from which runTestsParallel is called.

db.Exec("DROP TABLE IF EXISTS " + tableName)
dbt := &DBTest{t, db}
test(dbt, tableName)
dbt.db.Exec("DROP TABLE IF EXISTS test")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remaining reference to table test.

db.Exec("DROP TABLE IF EXISTS " + tableName)
dbt := &DBTest{t, db}
test(dbt, tableName)
dbt.db.Exec("DROP TABLE IF EXISTS test")
Copy link
Contributor

Choose a reason for hiding this comment

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

Remaining reference to table test.

@dolmen
Copy link
Contributor

dolmen commented Oct 18, 2023

The two functions below could be useful to abstract the handling of the DB connection life cycle in a test:

func openDB(tb testing.TB, dsn string) *sql.DB {
        tb.Helper()
        db, err := sql.Open("mysql", dsn)
        if err != nil {
                tb.Fatalf("open %q: %v", path, err)
        }

        tb.Cleanup(func() {
                err := db.Close()
                if err != nil {
                        tb.Error("close DB:", err)
                }
        })

        return db
}

func openDBTest(t *testing.T, dsn string) *DBTest {
        tb.Helper()
        db := openDB(dsn)
        return &DBTest{t, db}
}

They could be used in both runTests and runTestsParallel and make their code much shorter.

@shogo82148
Copy link
Contributor Author

Could not speed up as much as I expected. Close it.

@shogo82148 shogo82148 closed this Dec 11, 2023
@shogo82148 shogo82148 deleted the introduce-run-tests-parallel branch December 11, 2023 14:46
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.

3 participants