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.

Sign up for GitHub

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

Jump to bottom

T350363 - Remove unnecessary waitForDisplayed() #506

Merged
merged 26 commits into from
Nov 13, 2023
Merged

T350363 - Remove unnecessary waitForDisplayed() #506

merged 26 commits into from
Nov 13, 2023

Conversation

RickiJay-WMDE
Copy link
Member

@RickiJay-WMDE RickiJay-WMDE commented Nov 3, 2023 •
edited by rti
Loading

@RickiJay-WMDE RickiJay-WMDE changed the title TEST Display T350363 - Remove unnecessary waitForDisplayed() Nov 6, 2023
@lorenjohnson
Copy link
Contributor

@RickiJay-WMDE I like this approach. I started by just keeping awaitDisplayed in place and commenting out the waitForDisplayed calls within it. In local runs I was finding some tests right away fail. Surprisingly, it looks like all the tests passed here on CI without any waitForDisplayed call.

Either way, I much prefer just removing the helper as you've done here, and I'll continue off of this branch and do test runs, adding back waitForDisplayed in each case where the tests fail locally.

@lorenjohnson
Copy link
Contributor

lorenjohnson commented Nov 8, 2023 •
edited
Loading

So everything passes for me locally now too. Will run a couple more times then do a proper PR review and likely approve.

That said, @RickiJay-WMDE do you see anything here that you are not comfortable pushing forward? Would you expect something to be flaky? And have you ran the full test suite on this branch from your own machine and see everything pass?

* The one test that was holding things up (test/specs/repo/search.ts#11) on previous runs seems to actually also fail without this change on my machine. I will make create a different PR to patch it (for now with a now with a browser.pause()), as I think it is breaking for me because my machine runs through the tests a lot fast than CI and somewhat faster than the Thinkpads.

lorenjohnson and others added 8 commits November 9, 2023 06:00
@lorenjohnson
Fixes
acd5ff7
@RickiJay-WMDE
Lint
6ff1916
@RickiJay-WMDE
Organize
f81b56a
@rti @RickiJay-WMDE
fix(github-actions): GHCR upload (#505)
b2ead8d 
* fix(github-actions): GITHUB_TOKEN access for GHCR upload

* fix(github-actions): docker image name

* chore: move if to job level

* fix: get built docker artifacts in ghcr upload job

* fix(github-actions): ghcr image name

* chore: run upload ghcr only on main and mw-?.?? branches

* fix: simplify job run check, we do not have regex here :(
@rti @RickiJay-WMDE
T349718 property list test (#493)
d85f004 
* test(property-list): reproduce new property not shown issue

* fix: configure cache and wait for it to timeout

* fix: lint

* fix: list of properties page test for more than 50 properties
@rti @RickiJay-WMDE
fix: change upgrade test 1.39 base version to latest (#508)
dac4f98 
* chore: change upgrade test 1.39 base version to latest

* fix: add missing env files
@RickiJay-WMDE
Merge branch 'main' into no-display
eec2e04
@RickiJay-WMDE
Lint
3acb30f
@RickiJay-WMDE
Copy link
Member Author

RickiJay-WMDE commented Nov 9, 2023 •
edited
Loading

Hmmm. special-version and special-property are failing for me now, are they working for you? @lorenjohnson

@lorenjohnson
Copy link
Contributor

Hmmm. special-version and special-property are failing for me now, are they working for you? @lorenjohnson

Yes, everything was passing on multiple runs but with the merge up of main need to re-run. Will report back.

@RickiJay-WMDE RickiJay-WMDE marked this pull request as ready for review November 13, 2023 09:50
@RickiJay-WMDE RickiJay-WMDE requested a review from a team November 13, 2023 09:50
lorenjohnson
lorenjohnson approved these changes Nov 13, 2023
View reviewed changes
Copy link
Contributor

@lorenjohnson lorenjohnson left a comment

Choose a reason for hiding this comment

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

LGTM

@RickiJay-WMDE RickiJay-WMDE merged commit 48eca72 into main Nov 13, 2023
26 checks passed
@RickiJay-WMDE RickiJay-WMDE deleted the no-display branch November 13, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorenjohnson lorenjohnson lorenjohnson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@RickiJay-WMDE @lorenjohnson @rti