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

389 - Enhance Test Stability with Improved Wait Mechanisms #390

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

Janell-Huyck
Copy link
Contributor

@Janell-Huyck Janell-Huyck commented Mar 7, 2024

Fixes #389

Adds Improved Wait Mechanisms to Feature Tests

We were having issues with flaky tests due to the tests proceeding asynchronously, doing things like testing for conditions that happen after a button was clicked without waiting for the page to reload after the button click.

This PR includes using facotries and db checks to reduce unnecessary time spent mimicking screen clicking, and wait mechanisms to ensure feature tests more reliably wait for pages to fully load before proceeding.

This PR has run its CI tests 10 times without failing, so it should fix the test flakiness introduced in the spec/feature/author_management directory.

@Janell-Huyck Janell-Huyck changed the title 389 - Enhance Test Stability with Improved Wait Mechanisms WIP 389 - Enhance Test Stability with Improved Wait Mechanisms Mar 7, 2024
@Janell-Huyck Janell-Huyck changed the title WIP 389 - Enhance Test Stability with Improved Wait Mechanisms 389 - Enhance Test Stability with Improved Wait Mechanisms Mar 9, 2024
@hortongn hortongn self-assigned this Mar 11, 2024
expect(page).to have_selector("input[name='book[author_first_name][]']", visible: true)

expect(page).to have_content('Add Author')
expect(page).not_to have_content('Add Artist')
Copy link
Member

Choose a reason for hiding this comment

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

@Janell-Huyck You still have these checks for 'Add Author' and 'Add Artist' in several places in this file even though your new add_author_or_artist_and_verify_field method also checks for them. Can you safely get rid of these expects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Part of what I learned with this pr is that the statement expect(page).to have_selector("input[name='book[author_first_name][]']" has the side-effect of causing Capybara to wait until these elements are loaded before proceeding.

The problems I was having have to do with Capybara proceeding to check things before pages have actually loaded. The best way I found to slow Capybara down is to add these checks in before I check what I'm actually needing to check.

Are you aware of another way to do this? I have tried to sprinkle this check in anywhere where Capybara might have been expected to either initially load or to reload a page due to some server-side Javascript actions. It will wait up to the default_wait_time for these things to load, but if they're there it proceeds immediately.

Copy link
Member

Choose a reason for hiding this comment

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

I was only referring to lines 62 & 63. They verify the "Add Author" or "Add Artist" buttons are there, but then right after that you call add_author_or_artist_and_verify_field, which does its own (second) check that "Add Author" or "Add Artist" are present.

So I'm wondering if expect(page).to have_content('Add Author') and expect(page).not_to have_content('Add Artist') are redundant because of add_author_or_artist_and_verify_field? Or are they needed to slow Capybara down?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see what you're saying. The helper method is checking that there's only one type of button, but it doesn't really care which one is there. This test here is more about making that the correct button type is present. The "book" model should have "author", and if it had "artist" that should fail. The add_author_or_artist_and_verify_field method will pass for either one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebasing... hang on

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation and the rebase. I'll merge when the build goes green. (might want to look into turning on parallel testing in CI)

@Janell-Huyck Janell-Huyck force-pushed the 389-fix-flaky-author-management-tests branch from 71653bf to da1de1a Compare March 12, 2024 20:58
@hortongn hortongn merged commit 0075411 into qa Mar 12, 2024
2 checks passed
@hortongn hortongn deleted the 389-fix-flaky-author-management-tests branch March 12, 2024 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky Tests - Author Management feature tests
2 participants