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

fix: Enhance test stability in drive_tests.js #993

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

AfolabiOlaoluwa
Copy link
Contributor

In the drive_tests.js file, I improved test stability by introducing steps in two test cases: "test drive to external link" and "test drive enabled by default; click link inside data-turbo='false'."

The added calls ensure that the respective elements (e.g., #drive_enabled_external and #drive_disabled) are present and visible before interacting with them, reducing the chances of element unavailability issues during test execution.

This resolves test failures where elements were not immediately accessible, leading to test timeouts.

In the drive_tests.js file, I improved test stability by introducing
steps in two test cases: "test drive to external link" and
"test drive enabled by default; click link inside data-turbo='false'."

The added  calls ensure that the respective elements (e.g., "#drive_enabled_external"
and "#drive_disabled") are present and visible before interacting with them, reducing the chances of
element unavailability issues during test execution.

This resolves test failures where elements were not immediately accessible, leading to test timeouts.
… navigation wait

This commit reverts one of the changes made in commit 641947d where I replaced 'nextBody' with 'await page.click(#drive_disabled)'. The reason for this reversion is to stick to explicit waiting for page navigation to ensure that the page has fully transitioned to its new state before making assertions. The 'nextBody' function is used to achieve this waiting behavior, ensuring the reliability of our tests in certain scenarios.
@afcapel
Copy link
Collaborator

afcapel commented Sep 12, 2023

@AfolabiOlaoluwa I don't think these changes are necessary. Playwright already automatically waits for an element to be available before performing an action like page.click.

For more details, see https://playwright.dev/docs/actionability

@afcapel afcapel closed this Sep 12, 2023
@AfolabiOlaoluwa
Copy link
Contributor Author

AfolabiOlaoluwa commented Sep 12, 2023

@AfolabiOlaoluwa I don't think these changes are necessary. Playwright already automatically waits for an element to be available before performing an action like page.click.

For more details, see https://playwright.dev/docs/actionability

@afcapel test cases aren't passing despite. Should we rather leave it hanging and not fixed? So I am surprised why it is closed. The PR is not solely about page.click in my opinion. page.click wasn't necessarily passing without awaiting the id before click. I reverted just one change out of three (3) cases in the test case. Can you check all the changes? 641947d

@afcapel
Copy link
Collaborator

afcapel commented Sep 13, 2023

@AfolabiOlaoluwa adding await to page.click is correct, what we don't need is the page.waitForSelector before page.click. If you can update the branch with those changes I'll merge 👍

@afcapel afcapel reopened this Sep 13, 2023
@AfolabiOlaoluwa
Copy link
Contributor Author

AfolabiOlaoluwa commented Sep 13, 2023

adding await to page.click is correct, what we don't need is the page.waitForSelector before page.click. If you can update the branch with those changes I'll merge

I think should have been the comments as a review of the PR rather than the PR being closed. I do think we need to be patient about closing PRs.

I have made amends as I agree with some of your opinion clearly listed out on #993 (comment), @afcapel.

@afcapel afcapel merged commit a5534a2 into hotwired:main Sep 14, 2023
1 check passed
@afcapel
Copy link
Collaborator

afcapel commented Sep 14, 2023

Thanks @AfolabiOlaoluwa 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants