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

[usage] Fix flakes by deleting records created by each test, not deleting all #10971

Merged
merged 1 commit into from
Jun 28, 2022

Conversation

easyCZ
Copy link
Member

@easyCZ easyCZ commented Jun 28, 2022

Golang will run tests from different files (and packages) concurrently (in seperate go-routines). This presents a problem for our test cleanup logic which tries to execute DELETE * FROM <table> to cleanup a test table.

Follows guidance from https://kevin.burke.dev/kevin/fast-parallel-database-tests/

When tests are run, we use a helper from dbtest.CreateWorkspaces or dbtest.CreateWorkspaceInstances to create records which have a test cleanup hook. This hook deletes created records by ID instead of deleting all. This is necessary because when tests run concurrently, we don't want to drop records possibly created by other tests.

There still remain a problem for tests which try to do ListAll without any constraints but we don't have any of these in our usage component, yet.

Description

Related Issue(s)

Fixes #

How to test

Unit tests

Release Notes

NONE

Documentation

NONE

Werft options:

  • /werft with-preview

@geropl
Copy link
Member

geropl commented Jun 28, 2022

@easyCZ Got a couple of these while testing:

    --- FAIL: TestListWorkspacesByID/not_found_id_returns_emtpy_results (0.00s)
        workspace_test.go:129: 
                Error Trace:    conn.go:73
                                                        workspace_test.go:129
                Error:          Received unexpected error:
                                Error 1040: Too many connections
                Test:           TestListWorkspacesByID/not_found_id_returns_emtpy_results
                Messages:       Failed to establish connection to DB. In a workspace, run `leeway build components/usage:init-testdb` once to bootstrap the DB.

@easyCZ
Copy link
Member Author

easyCZ commented Jun 28, 2022

@easyCZ Got a couple of these while testing:

    --- FAIL: TestListWorkspacesByID/not_found_id_returns_emtpy_results (0.00s)
        workspace_test.go:129: 
                Error Trace:    conn.go:73
                                                        workspace_test.go:129
                Error:          Received unexpected error:
                                Error 1040: Too many connections
                Test:           TestListWorkspacesByID/not_found_id_returns_emtpy_results
                Messages:       Failed to establish connection to DB. In a workspace, run `leeway build components/usage:init-testdb` once to bootstrap the DB.

Interesting, I wasn't able to reproduce these. I guess that happens when the tests create too many connection against the DB. Perhaps a connection pool on the app side can solve this.

Let me try with a larger sample (1k runs) to see if I can reproduce, to test a fix.

@roboquat roboquat added size/XL and removed size/L labels Jun 28, 2022
@easyCZ
Copy link
Member Author

easyCZ commented Jun 28, 2022

@geropl I've moved the test connection setup into the dbtest package, where we share a connection for all the tests. That fixes the issue of too many connections in tests when running with -count 100

@easyCZ
Copy link
Member Author

easyCZ commented Jun 28, 2022

Now the only ones which do not like to be run in parallel are the ones where we create raw objects, due to seed values of random generation of workspace name. I'm gonna drop these tests as they are no longer really relevant, they existed to validate we can read from the existing DB without issues but we've long been running in that state that the data in the unit tests are no longer up-to-date either.

// Licensed under the GNU Affero General Public License (AGPL).
// See License-AGPL.txt in the project root for license information.

package db_test
Copy link
Member Author

Choose a reason for hiding this comment

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

These tests were moved to a seperate file because they required using the dbtest.ConnectForTest but were previously in the db package. This would create a circular import. Nothing changed about the test.

@easyCZ
Copy link
Member Author

easyCZ commented Jun 28, 2022

There remains an issue with the test TestListWorkspaceInstancesInRange which uses a fixed query arguments and will likely fail when that test is run concurrently (that test repeated concurrently). While annoying, there isn't a good fix that wouldn't blow up this PR.
I'm willing to accept that concurrent runs of this test will fail for now and land the PR as is as it is already an improvement for the general level of flakiness.

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM, tested and works as expected as per this comment.

@roboquat roboquat merged commit 247205c into main Jun 28, 2022
@roboquat roboquat deleted the mp/usage-flakes branch June 28, 2022 13:26
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/XL team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants