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

Update annotations unit test #880

Merged
merged 12 commits into from
Aug 29, 2022
Merged

Update annotations unit test #880

merged 12 commits into from
Aug 29, 2022

Conversation

GiaJordan
Copy link
Contributor

@GiaJordan GiaJordan commented Aug 25, 2022

Builds on #862. When two instances of the tests are running, an error can arise if they try to modify the same synapse entity concurrently. Waiting before trying again resolves. Implement tenacity to jitter instances of the tests and manage competition for shared resources. Annotations unit test now uses single dataset and manifest.

@GiaJordan GiaJordan marked this pull request as draft August 25, 2022 20:10
@GiaJordan GiaJordan requested a review from linglp August 25, 2022 20:21
@GiaJordan GiaJordan marked this pull request as ready for review August 25, 2022 20:22
@linglp
Copy link
Collaborator

linglp commented Aug 25, 2022

@GiaJordan this looks good. But I am also wondering how did you figure it out that the sleep time should be 60 instead of 30? There's also this library here that we could use for retry

@GiaJordan GiaJordan changed the title delay restart of test when interacting with same element Remove annotations unit test Aug 26, 2022
@GiaJordan
Copy link
Contributor Author

@GiaJordan this looks good. But I am also wondering how did you figure it out that the sleep time should be 60 instead of 30? There's also this library here that we could use for retry

Thanks for including the new library! There just needs to be enough time for one instance of the test to run and finish interacting with the entities on synapse before another does so.

@GiaJordan GiaJordan changed the title Remove annotations unit test Update annotations unit test Aug 26, 2022
Copy link

@BrunoGrandePhD BrunoGrandePhD left a comment

Choose a reason for hiding this comment

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

🚀 Looks good to me!

If the issue persists despite enabling retries, you could also look into forcing the tests to run sequentially in GitHub Actions (docs).

I also noticed that redundant tests are run for PRs, which might be contributing to clashes between them when accessing Synapse (see screenshot below). This could be avoided by refining the on: field for the test.yml CI workflow (docs).

This is something you could tweak in this PR. For example, the following config would run the CI workflow on pushes to the main branch as well as all PRs. I haven't tested this snippet, so it might require some tweaking. If you do made this change, we should be see right away whether there are duplicate checks here.

on:
  push:
    branches: [main]
  pull_request: 

Screenshot of Redundant Tests

image

tests/conftest.py Show resolved Hide resolved
tests/test_store.py Show resolved Hide resolved
@GiaJordan
Copy link
Contributor Author

@BrunoGrandePhD thank you for reviewing!

I'll keep those fixes in mind if issues arise with the tests again!

@GiaJordan GiaJordan merged commit 0c135b5 into develop Aug 29, 2022
@GiaJordan GiaJordan deleted the develop-blank-annotations branch August 29, 2022 17:23
@linglp linglp mentioned this pull request Aug 29, 2022
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.

None yet

3 participants