Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Fix for pytest-asyncio bug #1260

Merged
merged 8 commits into from
Sep 6, 2022
Merged

Fix for pytest-asyncio bug #1260

merged 8 commits into from
Sep 6, 2022

Conversation

sanders41
Copy link
Contributor

@sanders41 sanders41 commented Sep 6, 2022

Purpose

The pytest-asyncio plugin has a bug where tests patched with unittest mocks aren't automatically recognized as async so pytest skips them. This fixes that issue.

Changes

  • Add the @pytest.mark.asyncio decorator to patched tests.

Checklist

  • Update CHANGELOG.md file
    • Merge in main so the most recent CHANGELOG.md file is being appended to
    • Add description within the Unreleased section in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.
    • Add a link to this PR at the end of the description with the PR number as the text. example: #1
  • Applicable documentation updated (guides, quickstart, postman collections, tutorial, fidesdemo, database diagram.
  • If docs updated (select one):
    • documentation complete, or draft/outline provided (tag docs-team to complete/review on this branch)
    • documentation issue created (tag docs-team to complete issue separately)
  • Good unit test/integration test coverage
  • This PR contains a DB migration. If checked, the reviewer should confirm with the author that the down_revision correctly references the previous migration before merging
  • The Run Unsafe PR Checks label has been applied, and checks have passed, if this PR touches any external services

Ticket

Fixes #1240

@sanders41 sanders41 marked this pull request as ready for review September 6, 2022 16:08
@sanders41 sanders41 added the run unsafe ci checks Triggers running of unsafe CI checks label Sep 6, 2022
@sanders41 sanders41 added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Sep 6, 2022
@sanders41 sanders41 added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Sep 6, 2022
@sanders41 sanders41 added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Sep 6, 2022
@@ -37,7 +38,7 @@ def test_shopify_access_request_task(
merged_graph = shopify_dataset_config.get_graph()
graph = DatasetGraph(merged_graph)

v = graph_task.run_access_request(
v = await graph_task.run_access_request(
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@pattisdr pattisdr left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this change and nailing down the root cause!

@sanders41 sanders41 added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Sep 6, 2022
@pattisdr
Copy link
Contributor

pattisdr commented Sep 6, 2022

will wait until this is no longer in flux ^

@sanders41
Copy link
Contributor Author

Thanks @pattisdr I hope I caught them all with this last one, but we will see 😄

@sanders41
Copy link
Contributor Author

Just 1 test left failing. @adamsachs or @galvana can one of you point me to what the reason may be? I don't think any change I made should have affected this test. https://github.com/ethyca/fidesops/runs/8213079793?check_suite_focus=true#step:7:198

@pattisdr
Copy link
Contributor

pattisdr commented Sep 6, 2022

I wonder if there's a collision in the privacy request id.

The test is failing in an area that sends high-level statistics to fideslog so we can understand how graphs change in between runs on a single privacy request if the privacy request is retried from where it failed, or is resumed from a paused state. The collections it thinks it has already processed are mailchimp collections but this is a sendgrid test.

My guess is that the privacy request id we generated happened to overlap with a privacy request created in another test. I'm guessing a restart will fix this, but we should probably move to a privacy request fixture/do better about deleting privacy requests created in testing/increase the number of possible privacy request ids generated here in the future.

privacy_request = PrivacyRequest(
            id=f"test_saas_access_request_task_{random.randint(0, 1000)}"
        )
already_processed_access_collections = ['mailchimp_instance:member', 'mailchimp_instance:messages', 'mailchimp_instance:conversations']
current_graph = {'__ROOT__:__ROOT__': {}, '__TERMINATE__:__TERMINATE__': {'sendgrid_instance:contacts': []}, 'sendgrid_instance:contacts': {'__ROOT__:__ROOT__': ['__ROOT__:__ROOT__:email->sendgrid_instance:contacts:email']}}
added_edges = ['__ROOT__:__ROOT__:email->sendgrid_instance:contacts:email']

@sanders41
Copy link
Contributor Author

Nice find @pattisdr a collision seems like a good possibility. I restarted the tests.

@sanders41
Copy link
Contributor Author

Previous one passed with a rerun, but a different failure https://github.com/ethyca/fidesops/runs/8213447951?check_suite_focus=true#step:7:124. This one looks different, but again I don't think any change here would have touched it.

@sanders41 sanders41 added run unsafe ci checks Triggers running of unsafe CI checks and removed run unsafe ci checks Triggers running of unsafe CI checks labels Sep 6, 2022
@pattisdr
Copy link
Contributor

pattisdr commented Sep 6, 2022

I agree, I doubt that failure is related

@sanders41
Copy link
Contributor Author

@pattisdr debugging code is removed so this should be ready now.

@pattisdr pattisdr merged commit da31f5d into main Sep 6, 2022
@pattisdr pattisdr deleted the ps-async-test branch September 6, 2022 21:45
sanders41 pushed a commit that referenced this pull request Sep 22, 2022
Co-authored-by: Paul Sanders <pau@ethyca.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
run unsafe ci checks Triggers running of unsafe CI checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Marked Async Tests Are Skipped
2 participants