Tentative fix for flaky specs #3141

kennyadsl · 2019-03-13T17:23:25Z

Description

This PR is an attempt to fix flaky specs:

For more info it's better to take a look at individual commits.

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message

kennyadsl · 2019-03-14T16:51:21Z

This is passing, I rerun this workflow several times and it is always green (first failure is due to a force push, not a real failure). I think we can try to merge this.

aitbw

Ran the spec several times locally with no failures whatsoever, 🙏

kennyadsl · 2019-03-15T11:31:52Z

It failed again on the CI :(

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

kennyadsl · 2019-03-20T09:39:06Z

By looking at the last failure on CircleCI it's evident what happens:

The page fails in this state (at least when the screenshot is taken):

and the failure says:

Failure/Error: expect(page).to have_css(".select2-search-choice", text: taxon.name)
  expected to find visible css ".select2-search-choice" with text "Clothing" but there were no matches. Also found "Ruby on Rails", which matched the selector but not all filters.

This is the failing code:

solidus/core/lib/spree/testing_support/capybara_ext.rb

Lines 93 to 98 in 7ca285d

    
           def select_select2_result(value) 
        
             # results are in a div appended to the end of the document 
        
             within_entire_page do 
        
               page.find("div.select2-result-label", text: /#{Regexp.escape(value)}/i, match: :prefer_exact).click 
        
             end 
        
           end

page.find should wait for 10 (Capybara.default_max_wait_time) seconds, so I don't think it's an issue related to the page waiting for div to be populated correctly. Also, if Clothing were not found and clicked in the results div, Capybara would raise an exception. So for some reason, that result is found, clicked but it's not doing anything. I'll put some more debugging code, so it will be more clear what's happening.

jacobherrington

👍 This works for now.

By checking that select2 has correctly been updated before submitting the form, we ensure that we are passing the right data to the server. I also update the portion of the page where we look for the flash message so it will check for it after the element will be visible on the page (it is displayed after page load with js).

It looks like sometimes active is not recognized as a real method of Spree::StockLocation::ActiveRecord_Relation. Here's an example of the flaky spec failing: https://circleci.com/gh/nebulab/solidus/1701

tvdeyen

👍 for the select2 fixes. This library causes so many troubles. We really should think about removing it sooner or later.

The second spec should be rewritten IMO

tvdeyen · 2019-03-26T17:39:50Z

core/spec/models/spree/stock/location_filter/active_spec.rb


+          without_partial_double_verification do
+            expect(stock_locations).to receive(:active) { active_stock_locations }
+          end


Hmm, I am not a huge fan of these kind of specs. Could we rewrite the spec into something that actually test the result instead of stubbing the implementation?

Create two stock locations, one active, one not. Expect the active one is the result.

jacobherrington · 2019-03-26T18:43:25Z

+1 for getting rid of select2. Something I'd love to tackle if I had the time.

tvdeyen · 2019-03-26T19:33:58Z

+1 for getting rid of select2. Something I'd love to tackle if I had the time.

@jacobherrington Awesome! keep in mind, though, that we already only using select2 for multiple selects and autocomplete. Normal selects are already browser selects. John looked into an alternative, but didn’t finished his work.

In order to avoid stubbing the implementation. This could have benefits in terms of speed but everything is more complex to read and it's causing flaky specs in combination with verifying partial doubles.

This is not releated with this PR but I think we should change this code here to discourage adopting a practice that we don't want to see into other specs.

kennyadsl · 2019-03-27T09:49:41Z

@tvdeyen I've done the suggested changes to specs. I also changed Stock Location Sorter specs that I wrongly used as a reference when writing these ones.

kennyadsl · 2019-03-27T09:51:54Z

Aaaaand... there's another flaky: https://circleci.com/gh/nebulab/solidus/1726#tests/containers/2

tvdeyen · 2019-03-27T10:02:50Z

These feature specs are all written very poorly. One does not expect database changes in a feature spec. One expects changes in the request outcome (html). This particular one should be easily fixed by waiting for a string present on the screen before making that assertion about a change in the database (that happen in another thread and therefore can’t be predictable)

They rely on the database to be filled to check things happened correctly. This is a bad practice and we can easily check the same thing by checking the output of the page. This should fix this flaky spec: https://circleci.com/gh/nebulab/solidus/1726#tests/containers/2

tvdeyen

Awesome. Thank you!

kennyadsl self-assigned this Mar 13, 2019

kennyadsl added the Code Review Needed label Mar 14, 2019

kennyadsl marked this pull request as ready for review March 14, 2019 16:51

aitbw approved these changes Mar 14, 2019

View reviewed changes

kennyadsl force-pushed the kennyadsl/fix-flacky-specs branch from 55618c9 to e49cde5 Compare March 20, 2019 15:37

jacobherrington approved these changes Mar 21, 2019

View reviewed changes

kennyadsl added 2 commits March 26, 2019 14:06

Turn off partial double verification for StockLocation#active

967e2f5

It looks like sometimes active is not recognized as a real method of Spree::StockLocation::ActiveRecord_Relation. Here's an example of the flaky spec failing: https://circleci.com/gh/nebulab/solidus/1701

kennyadsl force-pushed the kennyadsl/fix-flacky-specs branch from e49cde5 to 967e2f5 Compare March 26, 2019 13:07

tvdeyen reviewed Mar 26, 2019

View reviewed changes

kennyadsl added 2 commits March 27, 2019 10:44

Rewrite Stock Location Filter specs

f9f4e06

In order to avoid stubbing the implementation. This could have benefits in terms of speed but everything is more complex to read and it's causing flaky specs in combination with verifying partial doubles.

Rewrite Stock Location Sorter specs

5c8fcff

This is not releated with this PR but I think we should change this code here to discourage adopting a practice that we don't want to see into other specs.

kennyadsl force-pushed the kennyadsl/fix-flacky-specs branch from fc8e82f to b5529d3 Compare March 27, 2019 10:29

kennyadsl force-pushed the kennyadsl/fix-flacky-specs branch from b5529d3 to b7b918a Compare March 27, 2019 10:30

kennyadsl requested a review from tvdeyen March 27, 2019 10:53

tvdeyen approved these changes Mar 27, 2019

View reviewed changes

kennyadsl merged commit 767c238 into solidusio:master Mar 27, 2019

kennyadsl deleted the kennyadsl/fix-flacky-specs branch March 27, 2019 10:57

kennyadsl mentioned this pull request Mar 27, 2019

When editing prices keep the currency locked. #3150

Merged

4 tasks

jacobherrington removed the Code Review Needed label Mar 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tentative fix for flaky specs #3141

Tentative fix for flaky specs #3141

kennyadsl commented Mar 13, 2019 •

edited

Loading

kennyadsl commented Mar 14, 2019

aitbw left a comment

kennyadsl commented Mar 15, 2019

kennyadsl commented Mar 20, 2019

jacobherrington left a comment

tvdeyen left a comment

tvdeyen Mar 26, 2019

jacobherrington commented Mar 26, 2019 •

edited

Loading

tvdeyen commented Mar 26, 2019

kennyadsl commented Mar 27, 2019

kennyadsl commented Mar 27, 2019

tvdeyen commented Mar 27, 2019

tvdeyen left a comment

Tentative fix for flaky specs #3141

Tentative fix for flaky specs #3141

Conversation

kennyadsl commented Mar 13, 2019 • edited Loading

kennyadsl commented Mar 14, 2019

aitbw left a comment

Choose a reason for hiding this comment

kennyadsl commented Mar 15, 2019

kennyadsl commented Mar 20, 2019

jacobherrington left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen Mar 26, 2019

Choose a reason for hiding this comment

jacobherrington commented Mar 26, 2019 • edited Loading

tvdeyen commented Mar 26, 2019

kennyadsl commented Mar 27, 2019

kennyadsl commented Mar 27, 2019

tvdeyen commented Mar 27, 2019

tvdeyen left a comment

Choose a reason for hiding this comment

kennyadsl commented Mar 13, 2019 •

edited

Loading

jacobherrington commented Mar 26, 2019 •

edited

Loading