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

Attempt to fix flaky specs #3278

Merged
merged 3 commits into from
Aug 9, 2019

Conversation

kennyadsl
Copy link
Member

@kennyadsl kennyadsl commented Jul 19, 2019

Description
This is an attempt to fix a couple of flaky specs that we have:

Using a more standard way of checking HTML elements state for Capybara could fix the issue since, using the current code, it could not be waiting for the element to change state/appear on the page.

Labeled as WIP. I'd like to re-run specs a lot of times before merge. Progress can be monitored here:

https://circleci.com/gh/nebulab/workflows/solidus/tree/kennyadsl%2Ffix-flaky-attempt

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
    - [ ] I have updated Guides and README accordingly to this change (if needed)
    - [ ] I have added tests to cover this change (if needed)

@kennyadsl kennyadsl added the WIP label Jul 19, 2019
@kennyadsl kennyadsl self-assigned this Jul 19, 2019
@kennyadsl kennyadsl force-pushed the kennyadsl/fix-flaky-attempt branch 2 times, most recently from 8b5effc to 19a8552 Compare August 7, 2019 14:58
Copy link
Contributor

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

Good clean up!


expect(find(".flash")).to have_text "Product \"#{product.name}\" has been successfully updated!"
within('.flash') do
expect(page).to have_content("Product \"#{product.name}\" has been successfully updated!")
Copy link
Contributor

@mdesantis mdesantis Aug 7, 2019

Choose a reason for hiding this comment

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

What about taking advantage of this change in order to improve string interpolation avoiding escaping double quotes? I was thinking something like this:

%(Product "#{product.name}" has been successfully updated!)

This is an attempt to fix a flaky spec that we have:

https://circleci.com/gh/nebulab/solidus/2687#tests/containers/1

Using a more standard way of checking HTML elements state for Capybara
could fix the issue since it could not be waiting for the button to
change state with the current code.
@kennyadsl kennyadsl removed the WIP label Aug 8, 2019
This is an attempt to fix a flaky spec:

https://circleci.com/gh/nebulab/solidus/2686#tests/containers/1

Probably using this method, Capybara will wait for the page to
display the flash message before making the expectation fail.
Without this line we have a flaky spec probably due to select2 not closing
its fixed overlay correctly.

We've notice that the submit button wasn't pressed at all when we had
failures. This means something is preventing the click to happen.
The select2 plugin has a lot of code that interacts with other page
elements preventing clicks so that could be the root cause.

Clicking anywhere in the page before submit apparently solves the issue.
Copy link
Contributor

@jacobherrington jacobherrington left a comment

Choose a reason for hiding this comment

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

Thanks for leaving the comment to explain the workaround, LGTM!

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

🎉 thank you @kennyadsl

@kennyadsl kennyadsl merged commit 349aa09 into solidusio:master Aug 9, 2019
@kennyadsl kennyadsl deleted the kennyadsl/fix-flaky-attempt branch August 9, 2019 09:15
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.

5 participants