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

front: fix flaky e2e test 008 #10475

Closed
wants to merge 4 commits into from
Closed

front: fix flaky e2e test 008 #10475

wants to merge 4 commits into from

Conversation

Maymanaf
Copy link
Contributor

@Maymanaf Maymanaf commented Jan 21, 2025 •
edited by anisometropie
Loading

No description provided.

@github-actions github-actions bot added the area:front Work on Standard OSRD Interface modules label Jan 21, 2025
@emersion
Copy link
Member

The issue might be this.timetableTrains.count(): when the list is virtualized, off-screen train HTML elements don't even exist so might not get counted? Hm, but that should only result in less trains being tested, not more tests failures?

@Maymanaf
Copy link
Contributor Author

The issue might be this.timetableTrains.count(): when the list is virtualized, off-screen train HTML elements don't even exist so might not get counted? Hm, but that should only result in less trains being tested, not more tests failures?

The count works fine, the issue is with selecting the train card when it's not loaded in the DOM

@Maymanaf Maymanaf force-pushed the aba/e2e-fix-flaky-e2e-test-008 branch 2 times, most recently from 1439b48 to 8d8843f Compare January 21, 2025 15:26
@Maymanaf Maymanaf marked this pull request as ready for review January 21, 2025 15:26
@Maymanaf Maymanaf requested a review from a team as a code owner January 21, 2025 15:26
@Maymanaf Maymanaf requested review from emersion and Yohh January 21, 2025 16:48
@clarani
Copy link
Contributor

clarani commented Jan 22, 2025

The commit has been reverted here, maybe we should re-add it ? @anisometropie

@Maymanaf Maymanaf force-pushed the aba/e2e-fix-flaky-e2e-test-008 branch from 8d8843f to 2634e9e Compare January 22, 2025 15:30
@emersion
Copy link
Member

Indeed, both commits from #10474 need to be re-applied to check whether the issue is indeed fixed. (Since it was flaky, need to test multiple times to ensure it doesn't succeed by chance.)

emersion
emersion reviewed Jan 23, 2025
View reviewed changes
front/tests/pages/op-timetable-page-model.ts Outdated
@@ -94,7 +94,7 @@ class OperationalStudiesTimetablePage extends CommonPage {

// Verify that simulation results are displayed
async verifySimulationResultsVisibility(): Promise<void> {
await this.page.waitForLoadState('networkidle');
await this.page.waitForLoadState('networkidle', { timeout: 60_000 });
Copy link
Member

Choose a reason for hiding this comment

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

60s is a very long time, do we really need it? I don't think #10371 increases the simulation time.

emersion
emersion reviewed Jan 23, 2025
View reviewed changes
front/tests/pages/op-timetable-page-model.ts Outdated
const trainElementHandle = this.timetableTrains.nth(currentTrainIndex);

await trainElementHandle.scrollIntoViewIfNeeded();
await trainElementHandle.waitFor();
Copy link
Member

Choose a reason for hiding this comment

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

Does waitFor() really do anything here? I'd assume scrollIntoViewIfNeeded() to be enough to make the element visible.

emersion
emersion reviewed Jan 23, 2025
View reviewed changes
front/tests/pages/op-timetable-page-model.ts Outdated
const trainButton = OperationalStudiesTimetablePage.getTrainButton(
this.timetableTrains.nth(currentTrainIndex)
);
const trainElementHandle = this.timetableTrains.nth(currentTrainIndex);
Copy link
Member

Choose a reason for hiding this comment

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

Kind of orthogonal to this PR, but it's nicer to use for (const trainElement of await this.timetableTrains.all()) instead of a counter.

@anisometropie anisometropie force-pushed the aba/e2e-fix-flaky-e2e-test-008 branch from 2634e9e to 08c13fb Compare January 28, 2025 16:20
@anisometropie anisometropie requested a review from a team as a code owner January 28, 2025 16:20
@anisometropie
Copy link
Contributor

anisometropie commented Feb 3, 2025 •
edited
Loading

pushed back in my commits for #10377, E2e are running ok

@Maymanaf Maymanaf force-pushed the aba/e2e-fix-flaky-e2e-test-008 branch from 08c13fb to 92d65e8 Compare February 5, 2025 10:35
@Maymanaf Maymanaf force-pushed the aba/e2e-fix-flaky-e2e-test-008 branch from 92d65e8 to 2b1016c Compare February 5, 2025 10:36
@Maymanaf
Copy link
Contributor Author

Maymanaf commented Feb 7, 2025

Fixed in this PR

@Maymanaf Maymanaf closed this Feb 7, 2025
@Maymanaf Maymanaf deleted the aba/e2e-fix-flaky-e2e-test-008 branch March 3, 2025 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emersion emersion emersion left review comments

@Yohh Yohh Awaiting requested review from Yohh

Assignees
No one assigned
Labels
area:front Work on Standard OSRD Interface modules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Maymanaf @emersion @clarani @anisometropie