chanzuckerberg / single-cell-data-portal

The data portal supporting the submission, exploration, and management of projects and datasets to cellxgene.
MIT License
64 stars 14 forks source link

bug(functional tests): API key management tests are flakey due to race condition #6198

Closed Bento007 closed 6 months ago

Bento007 commented 1 year ago

Describe the bug

In tests/functional/backend/corpora/test_api_key.py, line 23 returns 200 instead of 404. This is because multiple instance of the functional tests are running and they all use the same auth0 test user.

To Reproduce

It is difficult to reproduce but it happens often enough to be a problem. Run functional tests in multiple threads and it will happen eventually. The specific test suite is TestApiKey.

Expected behavior

Tests running in separated rdev environment should not affect each other. Since all of the rdev environments are using the same auth0 test user something need to be changes to either share it, or eliminated the needs to access the shared resource.

Environment

Possible Solutions:

  1. Add a GHA concurrency lock for rdev functional tests. +(Quick and easy to implement) -(will slow down development)
  2. Mock out all of the auth0 calls for functional tests. - (Significant test refactor.) -(will need additional testing for specifically the auth0 integration)
  3. generate a new user per functional test. -(more reliance on auth0) -(will require some permission changes to auth0 which may not work since single-cell-infra is broken when it comes to updating auth0.) +(increase isolation of tests),
  4. Skip the TestApiKey in rdev. -(this means less test coverage for rdev. Future development of this area of code will be more difficult.) +(less code coverage for rdev environments for this feature.)
Bento007 commented 1 year ago

Solution 4 with appropriate comments that tells the user to uncomment this code when working on the feature is sufficient. A comment should live near the implementation as well as the tests so the developer has a higher likelyhood of remembering to uncomment the test.

metakuni commented 6 months ago

Closing, since @Bento007 noted that solution 4 is sufficient for now. We'll reopen/revisit if necessary.