-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(test): Add frontend integration test manual run #8767
Conversation
Skipping CI for Draft Pull Request. |
/assign @chensun |
@@ -0,0 +1,24 @@ | |||
# Frontend integration test |
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.
Thank you for adding the additional running mode, and the documenting the usage.
/cc @jlyaoyuli in case frontend team has a similar or better way to run FE integration test in custom deployment.
browser.execute('document.querySelector(".tableRow a").click()'); | ||
browser.waitUntil(() => { | ||
return new URL(browser.getUrl()).hash.startsWith('#/runs/details'); | ||
}, waitTimeout); |
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.
Waiting for the run details page to load is technically correct but doesn't change anything in the current setup, because $('button=Config').waitForVisible();
below would also ensure the page to load--unless there's a config button on run list page.
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.
To add my 5 cents: waiting for the page is more explicit. I reverted the change as you suggested above.
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.
@gkcalat Absolutely agree explicit is better, and that's what I meant by "technically correct". For this PR, I think it's better to focus on what it claims, the effective change in the helloworld.spec.js
can be reduced into one-line change of waiting for 90 seconds before clicking into "config" tab, and I was a bit skeptical whether that would fix the test failure we saw. For that reason I suggested we don't make this not-so-relevant change.
If we were to change this file in a meaningful way again, say in your large PR, I would support waiting explicitly on the page load.
/lgtm Thanks! /cc @zijianjoy @jlyaoyuli |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chensun The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Description of your changes:
The goal of this PR is to improve debugging experience for the frontend integration test.
Checklist: