-
Notifications
You must be signed in to change notification settings - Fork 212
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
OPIK-501 Basic experiment items CRUD tests #865
Conversation
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.
Couple of minor comments for potential follow-ups, but not blockers. Everything else LGTM.
self.page.wait_for_timeout(500) | ||
ids.extend(self.get_all_dataset_items_on_current_page()) | ||
|
||
return ids |
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.
Minor: mentioned before, but please add a pre-commit hook to format the Python code within this tests_end_to_end
folder, so among other things, it adds an empty line at the end of each file. Use the SDK linting as reference.
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.
good point, i went ahead and added full linting functionality, along with a workflow file to check on every PR. applied it as part of this PR, everything looks good. will add more bits in the future when doing improvements (i.e. clean typing on everything like the SDK code currently has)
@@ -48,6 +48,7 @@ def insert_dataset_items_ui(page: Page, dataset_name, items_list): | |||
|
|||
for item in items_list: | |||
dataset_items_page.insert_dataset_item(json.dumps(item)) | |||
time.sleep(0.2) |
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.
Any particular reason to throttle the insertion pace here? The recommended alternative is inserting dataset items in batches, so you can insert the whole items_list
in one go.
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.
The main bit of flakiness that's currently present in these tests is one dataset item you insert via the UI not registering if you switch away too quickly. Added a small delay to try and prevent that and see if it works. As far as I know there is no way to add a list of dataset items via the UI, only one at a time, which is why I do it this way here. When inserting via the SDK i just pass the entire list in one go
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.
The linting is great, but apologies because I confused you. I meant to send it in a separated PR, otherwise is hard to review. Let's separate this into:
- Basic experiment items CRUD tests
- All the linting infra and the updated files.
e8252ac
to
357fb30
Compare
@andrescrz all good now, reverted the linting fixes and will get them in via a different PR |
Details
Tests proper creation and deletion of experiment items, on both UI and backend.
Coverage:
Notes: