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

Cop idea: disallow calling have_no_XXX immediately after visit (Capybara) #7

Open
boris-petrov opened this issue Jul 22, 2022 · 7 comments · May be fixed by #36
Open

Cop idea: disallow calling have_no_XXX immediately after visit (Capybara) #7

boris-petrov opened this issue Jul 22, 2022 · 7 comments · May be fixed by #36
Labels

Comments

@boris-petrov
Copy link
Contributor

Note the following code:

visit 'google.com'
expect(page).to have_no_link

Will pass... sometimes. It's a race-condition as the page may or may not have loaded when execution reaches the second line. It would be nice to have a cop that checks for these cases.

Not sure if it should also handle:

visit 'google.com'
within 'div'
  expect(page).to have_no_link
end

In some sense the page has already (at least partially) loaded because of the within check but on the other hand perhaps always having a positive check before the negative for sanity is better?

@boris-petrov boris-petrov changed the title Cop idea: disallow calling have_no_XXX immediately after visit Cop idea: disallow calling have_no_XXX immediately after visit (Capybara) Jul 22, 2022
@pirj pirj transferred this issue from rubocop/rubocop-rspec Dec 29, 2022
@ydah ydah added the cop label Feb 16, 2023
ydah added a commit that referenced this issue Apr 3, 2023
Resolve: #7

This PR adds a new `Capybara/NegationMatcherAfterVisit` cop.
@ydah ydah linked a pull request Apr 3, 2023 that will close this issue
11 tasks
ydah added a commit that referenced this issue Apr 3, 2023
Resolve: #7

This PR adds a new `Capybara/NegationMatcherAfterVisit` cop.
@pirj
Copy link
Member

pirj commented Apr 3, 2023

I wonder what this means exactly:

Capybara waits upon failed predicates/assertions
Capybara's RSpec matchers, however, are smart enough to handle either form.

As you say, "it would sometimes pass" means that it would fail most of the time. But what is the purpose of a spec that fails most of the time?

Adding a positive expectation before a negative one doesn't mean that the negative would always be correct.
E.g. a positive expectation could be against a statically loaded element, while the following negative against a dynamically loaded one. So this suggestion is not a panacea, and may instil false hope in the spec correctness.

Not to mention, the same behaviour applies to click_link that may cause a partial page update, not just visit.

Wondering if using cuprite/ferrum backend would solve the race condition issues.

@ydah
Copy link
Member

ydah commented Apr 24, 2023

This issue that this cop aims to solve is that when a negative check is performed on elements that are dynamically loaded, there is a possibility that the check will always succeed, even if the loading is slow. In other words, when the loading is slow, the check returns success immediately, and the motivation is to prevent not noticing failures in testing.
If there is anything that I might have missed, please let me know.

@pirj
Copy link
Member

pirj commented Apr 24, 2023

the following two statements are not equivalent, and you should always use the latter!

# Given use of a driver where the page is loaded when visit returns
# and that Capybara.predicates_wait is `true`
# consider a page where the `a` tag is removed through AJAX after 1s
visit(some_path)
!page.has_xpath?('a')  # is false
page.has_no_xpath?('a')  # is true

Capybara waits upon failed predicates/assertions, therefore has_no_xpath? does not return false immediately

Why is ‘has_no_xxx’ unsafe then?

@boris-petrov
Copy link
Contributor Author

@pirj check my first example in my original post. The second line could execute before the first one has loaded anything. Any empty page won't have a link and that check would pass. That obviously is not correct because google.com has links on it.

@pirj
Copy link
Member

pirj commented Apr 24, 2023

So we are talking about the following risk:

  1. We’ve had a page with no X, and have a test to cover that with ‘have_no_X’
  2. We’ve later made everything on the page to load dynamically
  3. We’ve added X
    … and the test keeps passing 💥
    Except occasionally failing due to quicker processing of JS.

I agree with the suggestion. But a system built this way feels whacky.

As a devil advocate, we can imagine some weird JS run on timer that would add and remove elements from the page (making some async server requests along the way) and in that case there’s no “complete” state of the page when we can make assertions against it.
So it seems we’re in a situation when we have to consider it complete at some point anyway.
This makes Culprite etc orthogonal to the problem in question.

@ydah
Copy link
Member

ydah commented Apr 27, 2023

The reason I wanted to implement this cop was to detect offenses in cases like the following.

In an application, the buttons to be displayed change depending on the user's permissions.
In such cases, the following tests may be written. What do you think of these cases?

context 'when current user has admin role' do
  let(:user) { create(:user, role: :admin) }

  it 'shows management button' do
    visit foo_page_url

    expect(page).to have_button 'Management'
  end
end

context 'when current user has not admin role' do
  let(:user) { create(:user, role: :common) }

  it 'does not show management button' do
    visit foo_page_url

    expect(page).not_to have_button 'Management' # If the display is slow here, it could always succeed 💥
  end
end

@boris-petrov
Copy link
Contributor Author

Yep, exactly my point - the second case has a bad race-condition that pretty much leads to your test not testing what you meant - it practically tests nothing (also, I think to have_no_button is better than not_to have_button).

ydah added a commit that referenced this issue Oct 2, 2023
Resolve: #7

This PR adds a new `Capybara/NegationMatcherAfterVisit` cop.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants