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

Initial playwright setup #1665

Merged
merged 79 commits into from
Jul 20, 2023
Merged

Initial playwright setup #1665

merged 79 commits into from
Jul 20, 2023

Conversation

kcpevey
Copy link
Contributor

@kcpevey kcpevey commented Feb 28, 2023

Reference Issues or PRs

First setup of #1653

What does this implement/fix?

Put a x in the boxes that apply

  • New feature (non-breaking change which adds a feature)

Testing

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

Any other comments?

@kcpevey
Copy link
Contributor Author

kcpevey commented Mar 9, 2023

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.

@costrouc
Copy link
Member

@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.

@kcpevey
Copy link
Contributor Author

kcpevey commented Mar 16, 2023

@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
Copy link
Contributor Author

kcpevey commented Mar 20, 2023

@costrouc @iameskild

  • When should the playwright tests run? I was thinking after a merge to develop or main?
  • Where should they run? I'm guessing on a test deployment of the current branch? Where can I find that? and the URL for it?

@costrouc
Copy link
Member

costrouc commented Mar 20, 2023

@kcpevey
Copy link
Contributor Author

kcpevey commented Mar 20, 2023

@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?

.github/workflows/kubernetes_test.yaml Outdated Show resolved Hide resolved
tests_e2e/playwright/README.md Outdated Show resolved Hide resolved
tests_e2e/playwright/README.md Show resolved Hide resolved
tests_e2e/playwright/README.md Show resolved Hide resolved
tests_e2e/playwright/conftest.py Outdated Show resolved Hide resolved
Comment on lines 431 to 433
import os

import dotenv
Copy link
Member

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(
Copy link
Member

@pmeier pmeier May 11, 2023

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

  1. Create the notebook
  2. Push it to a branch
  3. Put the branch name into the repo cloning
  4. 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.

@iameskild
Copy link
Member

It looks like this is nearly complete. @kcpevey @pmeier is there anything else that we need to do here (aside from passing CI and fixing the merge conflicts)?

@pmeier
Copy link
Member

pmeier commented Jun 19, 2023

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.

@kcpevey
Copy link
Contributor Author

kcpevey commented Jul 11, 2023

I'm going to try to carve out some time to get this wrapped up this week.

@aktech
Copy link
Member

aktech commented Jul 18, 2023

@kcpevey @pmeier @iameskild @costrouc I have changed the git clone to write file on the nebari instance via bash using cat and EOF to remove the dependency of having tests in a branch on github. I think this pull request is in good shape to be merged. I wanted to wrap this up as I might need this for creating some testing setup. Let me know if anyone have any objections/reviews/comments.

@aktech aktech requested a review from pmeier July 18, 2023 22:32
@kcpevey
Copy link
Contributor Author

kcpevey commented Jul 19, 2023

@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!

Copy link
Member

@pmeier pmeier left a 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.

tests_e2e/playwright/.env.tpl Outdated Show resolved Hide resolved
tests_e2e/playwright/README.md Show resolved Hide resolved
tests_e2e/playwright/README.md Outdated Show resolved Hide resolved
tests_e2e/playwright/README.md Show resolved Hide resolved
Copy link
Member

@pmeier pmeier left a comment

Choose a reason for hiding this comment

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

Thanks @kcpevey for the bulk of the work and @aktech for the polish. Happy with the result now. LGTM if CI is green!

Copy link
Member

@aktech aktech left a comment

Choose a reason for hiding this comment

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

thanks @kcpevey awesome work 🎉 and thanks @pmeier for persistence with this

@aktech aktech merged commit 9bcf67f into develop Jul 20, 2023
25 checks passed
@aktech aktech deleted the add_playwright branch July 20, 2023 14:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants