-
Notifications
You must be signed in to change notification settings - Fork 2
Create and configure new actions workflow for E2E tests #2721
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
Conversation
…m on PR creation or update.
….yaml to require passing unit tests before running the e2e tests. Update CONTRIBUTING.md with ci info. Update cypress config to rerun failures 1 time in run (headless) mode.
0623b22
to
af562d7
Compare
…or dependent builds. Remove unnecessary venv setup.
af562d7
to
2f52a82
Compare
7bb5931
to
cfe8788
Compare
a68ea80
to
956c896
Compare
… workflow instead of rebuilding. Minor modification to code-server-entry.sh for CI compatibility and safety checks.
fd31337
to
a3c4f14
Compare
…ver. Add wait loop to wait for code-server to be ready. Revert how we install the extension. Remove several troubleshooting steps and other unneeded changes.
a37fe66
to
b81e037
Compare
… flags set to true and always upload those artifacts regardless of pass/fail states when DEBUG flags are true.
b81e037
to
6856ed5
Compare
756212b
to
c35a8ac
Compare
…2707-publisher-e2e-ci
…2707-publisher-e2e-ci
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.
Thanks for taking on this work! The code changes look great, but I hit some problems when trying to validate the changes.
- When running
just dev
, the embedded-deployments tests (specificallyfastAPI in subdirectory of workspace
) failed w/
get iframe.webview.ready
assert expected iframe.webview.ready to exist in the DOM
- I retried the test and it worked.
- I then exited and ran
npx cypress run
and it ran fine - I then retried running
just dev
and it failed again at that same test.
What I was able to notice was that it looked like the publisher extension was not actively selected from the left taskbar. So, of course, the iframe wouldn't exist. I wonder if you might need to check that logic and perhaps select it again on failure? The button was visible and able to be clicked.
Since this PR includes activating the e2e runs within CI, can you take a look at this logic once again? Please let me know when you want me to test it again. Once I can get repetitive successful runs, even with an intermittent failure, I'll be happy to approve. It was just hitting this a second time during my verification, which makes me uneasy.
…2707-publisher-e2e-ci
Thanks for trying it @sagerb. I will take a look and see if adding that click back in helps. I may have accidentally dropped it with the helper changes. Most of my local testing was with the run command vs open, so it's possible I just missed that intermittent failure locally. |
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.
From further conversations:
bill.sager:
Tim, I hit failures twice running this locally, but I noted that it looked like the extension had not been clicked upon, which is required ahead of looking for the iframe. I've added further notes within the PR. Let me know when you want me to try it again. (Retry does work, but I did have it fail twice within 4 attempts.)
tim.yaeger:
Thanks Bill. I’m still working to get this one more consistently stable it’s proving to be a challenge. Did you have any of the other tests fail on you?
bill.sager:
No, that was the only one. Prior to the changes, the tests all ran successfully time and time again for me (locally).
tim.yaeger:
Ok, I’ll see if I can get this one more stable for a bit longer, but I mainly was asking because I currently have this particular test skipping in CI for a different permissions issue that only happens there and I wasn’t able to get to the bottom of that either. So at least for now the embedded test won’t affect the CI runs due to being skipped.
I do want to get both things fixed on this test, but thought I might just do it in a separate PR/ticket. I also have some new tests to add in another branch for our CC credentials.
bill: My theory (based on the test run recordings) is that the click on the Publisher icon isn't working to select the publisher views. I wonder if you might want to explore modifying the method in which the code activates the publisher button, to invoking the view -> Open View... menu, and then typing in Posit Publisher w/ a return. Maybe that is more reliable than the mouse click?
As long as you have those tests skipping in CI, and that you're planning on following up on the fixes, I'm good with proceeding forward getting this PR merged. Let me update the PR.
Approving the PR and leaving it in the hands of Tim to resolve. Thanks!
…2707-publisher-e2e-ci
…2707-publisher-e2e-ci
…2707-publisher-e2e-ci
…2707-publisher-e2e-ci
…2707-publisher-e2e-ci
Cypress E2E CI Setup and Test Reliability Improvements
I think I finally got this in a reliable enough state. I plan to squash the commits since there were so many as I was troubleshooting.
Closes #2707
Intent
Type of Change
Approach
embedded-deployments.cy.js
tests and they are being skipped for now in CI.Cypress.skipCI
can wrap anydescribe
orit
for this. The above tests will still run and pass locally however.User Impact
n/a
Automated Tests
embedded-deployments.cy.js
due to permissions issues in CI (see above)Directions for Reviewers
npx cypress run
ornpx cypress open
(orjust dev
for that matter) >> There may still be some flake on the embedded deploy tests running local that will need to be followed up on. Improve embedded_deployments.cy.js test issues #2872Checklist