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

Refactor and extend Playwright tests #2644

Merged

Conversation

viniciusdc
Copy link
Contributor

@viniciusdc viniciusdc commented Aug 27, 2024

Reference Issues or PRs

related to #2020
closes #2642

What does this implement/fix?

  • Refactor playwright main navigator class to allow better flexibility when extending test suit;
  • Add new conda-store UI class to test its accessibility and running;
  • Rewrote some internal conditional logic when locating objects within the jupyterlab workspace;
  • Add the capability to name video artifacts based on the executed test name.

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Any other comments?

I will open a follow-up PR to extend the conda-store test suit based on @kcpevey suggestion #2020 (comment)

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 28, 2024

Tests are failing due to the problem outlined in #2643, but we can still validate the playwright tests are now working:

video_test_login_logout.mp4
video_test_navbar_services.mp4
video_test_notebook.mp4
video_test_conda_store_ui.mp4

@viniciusdc viniciusdc added the needs: review 👀 This PR is complete and ready for reviewing label Aug 28, 2024
Copy link
Member

@marcelovilla marcelovilla left a comment

Choose a reason for hiding this comment

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

Awesome work @viniciusdc! I think this is a good first step in refactoring our tests and making them more extensible.

Regarding issue #2020, I'd say this PR does not necessarily closes it as based on the description, we also want to test environment creation and build deletion. I'm not saying we need to address that right away in this PR and I'll let you decide whether it should be closed or not.

@@ -172,7 +172,7 @@ jobs:
# create environment file
envsubst < .env.tpl > .env
# run playwright pytest tests in headed mode with the chromium browser
xvfb-run pytest --browser chromium
xvfb-run pytest --browser chromium --slowmo 300 --headed
Copy link
Member

Choose a reason for hiding this comment

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

@viniciusdc is there any downside of having the --slowmo flag here? Based on your commit messages, I'm guessing you added it to allow some time for the notebook to load.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only downside would be that all related playwright actions within a test (click a button, query, refresh etc) will take a slight increase in execution time which in return increases the time for the overall playwright pytest suit to complete -- though, I advice that this is essential for some components to load properly

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing the increase in execution time won't really be a significant issue. I'm more concerned with whether the appropriate way of dealing with components loading would be to use the wait_for_load_state (or similar) method directly within the playwright page.

I'll let you decide what's best and I don't think we need to implement everything in this PR anyways. Thanks Vini!

@viniciusdc
Copy link
Contributor Author

viniciusdc commented Aug 30, 2024

'd say this PR does not necessarily closes it as based on the description, we also want to test environment creation and build deletion.

@marcelovilla, good point; I will remove that closing tag and add a simple ref. Instead, I saw a note from @kcpevey on recent playwright tests added to conda-store; I was going to open a follow-up issue to extend this base class, as the main goal for this one was to at least catch the problem with starting the plugin at all.

@viniciusdc viniciusdc merged commit ed170cb into nebari-dev:develop Aug 30, 2024
9 of 10 checks passed
@viniciusdc viniciusdc deleted the 2642-refactor-playwright-tests branch August 30, 2024 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: review 👀 This PR is complete and ready for reviewing
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[BUG] - TimeoutError in Playwright Notebook tests
2 participants