-
Notifications
You must be signed in to change notification settings - Fork 68
[AL-4418] [AL-3976] Make dataset tests parallelizable #831
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
[AL-4418] [AL-3976] Make dataset tests parallelizable #831
Conversation
apollonin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a great start! Do you want to merge these changes right away?
| dataset.update(description=description) | ||
| assert dataset.description == description | ||
|
|
||
| dataset.delete() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to add a check for the deleted dataset here by querying for the dataset
with pytest.raises(SomeException):
client.get_dataset(dataset.uid)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This already exists on line 44.
| from copy import copy | ||
|
|
||
|
|
||
| def test_get_one_and_many_dataset_order(client): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work adding coverage for pagination!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt that part of the test_dataset was really testing pagination so I moved that here.
| def test_timeout_error(client, project): | ||
| with pytest.raises(labelbox.exceptions.TimeoutError) as excinfo: | ||
| query_str = """query getOntology { | ||
| project (where: {id: $%s}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how did it even work ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it timed out before this got evaluated.
| CAPTURE_DT_SCHEMA_ID = "cko8sdzv70006h2dk8jg64zvb" | ||
| PRE_COMPUTED_EMBEDDINGS_ID = 'ckrzang79000008l6hb5s6za1' | ||
| CUSTOM_TEXT_SCHEMA_NAME = 'custom_text' | ||
| CUSTOM_TEXT_SCHEMA_NAME = 'datarow_metadata_custom_text' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we want to use random schema name here for each test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is not needed anymore so I'll revert it, before we were creating and deleting this for each test, but that doesn't work with parallel tests.
| assert project.organization() == org | ||
| assert set(user.projects()) == user_projects.union({project}) | ||
| assert set(org.projects()) == org_projects.union({project}) | ||
| assert project in user_projects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch! It has been intermittently failing recently on staging and with your change now should be fine!
1f6c81d to
6177b59
Compare
Remove the assumption that only the current test is modifying data stores for dataset integration tests. Tested on staging.
