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

Fixed Rspec Checks Failure of assigned_articles_spec.rb #5654

Conversation

PR4NJ41
Copy link
Contributor

@PR4NJ41 PR4NJ41 commented Feb 13, 2024

What this PR does

This PR fixes Rspec checks failure which were failing due to assigned_articles_spec.rb file.

Addresses Issue #5652

Screenshots

Before:

After:
Screenshot 2024-02-14 at 12 10 47 AM

@PR4NJ41
Copy link
Contributor Author

PR4NJ41 commented Feb 13, 2024

@ragesoss
Here is the Preview:

After.mp4

@@ -21,9 +21,9 @@
# we need to use VCR to avoid getting stopped by WebMock
VCR.use_cassette('assigned_articles_view') do
visit "/courses/#{course.slug}/articles/assigned"
expect(page).to have_content('Nancy Tuana')
page.has_content?('Nancy Tuana')
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it simply removes the check for the expected article, so that if .has_content? returns false it doesn't matter and the test continues... whereas the expect line also relies on Capybara to wait a certain amount of time and continue checking for the content, so it ensure that the assigned articles list has rendered before moving on. I don't think will make it more reliable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I thought that the has_content waits till it is fully rendered.

@ragesoss
Copy link
Member

The test usually passes, so if you weren't able to replicate the failure locally, I suspect that this won't improve the test.

@@ -21,7 +21,7 @@
# we need to use VCR to avoid getting stopped by WebMock
VCR.use_cassette('assigned_articles_view') do
visit "/courses/#{course.slug}/articles/assigned"
page.has_content?('Nancy Tuana')
expect(page).to have_content('Nancy Tuana', wait:20)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not certain about how page.has_content? works, but have_content automatically waits and continues checking the page (up to a certain amount of time) until the expected content is found, so there's no need for the wait argument here.

@ragesoss
Copy link
Member

Note that this spec still failed on CI for this PR.

@PR4NJ41
Copy link
Contributor Author

PR4NJ41 commented Feb 13, 2024

@ragesoss The have_content is not waiting long enough to load the page. This page takes time to load depending on the server. So if the page has not loaded yet it will throw error of not found. To fix this i added wait, this will force it to wait for sometime to load.

@ragesoss
Copy link
Member

Hmm... the default wait time is pretty long; what makes you think that waiting even longer is the solution? Why does it sometimes take that long to load? My guess was that when it fails, it's not because of being slow to render, but that something else went wrong such as a request that failed or didn't return the expected data. But I guess that manual_update can take a while. The solution might instead be to adjust the test setup so that a potentially slow manual update is not needed.

@ragesoss
Copy link
Member

That 'manual_update' speculation is relevant for another flaky spec, but not this one. But it still seems related to too-slow network requests that rely on external services. I think the right fix is probably to identify the slow request, and then stub it (with a new method in request_helpers.rb) so that the test can run quickly without actually hitting the slow API.

@ragesoss ragesoss closed this Feb 15, 2024
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.

2 participants