-
Notifications
You must be signed in to change notification settings - Fork 95
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
Initial playwright setup #1665
Initial playwright setup #1665
Conversation
A big challenge that I don't have a great solution for is sometimes jlab opens with a popup already open (requesting a kernel for a notebook) and sometimes it doesn't. Since its a popup, the only thing we have access to are the popup dialog items. The only solution I can find is to wrap a try/except around getting a popup dialog item. If it doesn't exist, it will fail and that's the only way I have to know that the popup doesn't exist. Not great, but I tried many other options and nothing worked. |
@kcpevey I'd like to split this PR into smaller PRs after we merge this initial PR. We should limit the scope to replacing only the cypress tests as an MVP. If possible too I'd like to minimize the number of abstrations we create. Then we work on adding each smaller feature. This PR as is just isn't well scoped and hard to finish in a certain amount of time. |
@costrouc I completely agree. That's why I was hoping to get this first one merged before adding more tests. This just shows what I was hoping would be one "easy" integration test. I will add more details in the README on my vision of how the rest of the integration test suite would be filled out. I don't intend on removing cypress until we get a bunch of other PRs in and we're sure this is stable. |
|
@kcpevey I'd like the playwright tests to run in /~https://github.com/nebari-dev/nebari/blob/main/.github/workflows/kubernetes_test.yaml#L165-L169 and remove /~https://github.com/nebari-dev/nebari/blob/main/.github/workflows/kubernetes_test.yaml#L138-L163. |
Thanks! I think we should keep cypress for now and run both test suites until we have full coverage of the cypress tests with the playwright tests. Would you agree? |
tests_e2e/playwright/navigator.py
Outdated
import os | ||
|
||
import dotenv |
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 just different opinions on the style here. We usually don't scope imports by usage inside a file, i.e. if an import is only used in one function, it still goes at the top of the file. The only reason I use lazy imports is if the package in question is an optional dependency that might not be available, but is not needed for most parts of the module. That is not the case here. os
is from the standard library and dotenv
is a dev requirement.
I would prefer to put them at the top, but I won't block over this. Feel free to resolve without fix if you disagree with me.
|
||
def test_notebook(navigator): | ||
test_app = RunNotebook(navigator=navigator) | ||
test_app.nav.clone_repo( |
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.
So the general dev workflow would be
- Create the notebook
- Push it to a branch
- Put the branch name into the repo cloning
- Repeat 2. every time you make a change to the notebook
? That is quite inconvenient. Is there no better way to get this file into the cluster whether it is deployed locally or remotely?
In any case, we definitely need to document this workflow in the README.
Co-authored-by: Philip Meier <github.pmeier@posteo.de>
We need to at least resolve #1665 (comment) before merge. Some documentation is missing and possibly some workflow improvements for better DevX. There are a few more open comments, but nothing blocking. |
I'm going to try to carve out some time to get this wrapped up this week. |
@kcpevey @pmeier @iameskild @costrouc I have changed the git clone to write file on the nebari instance via bash using |
@aktech Thank you for taking this over! As for the reliance on a branch, that was just to show tests passing and not intended to be merged. Thank you improving things and pushing things through! |
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.
A few minor things inline that I'll fix in a second. Apart from them we also need to document that this needs a user to be present on the cluster and we can't do with root
. @aktech will add this. Afterwards, LGTM.
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.
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.
Reference Issues or PRs
First setup of #1653
What does this implement/fix?
Put a
x
in the boxes that applyTesting
Any other comments?