-
Notifications
You must be signed in to change notification settings - Fork 383
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
Improve tests result consistency #581
Conversation
In this commit, a deply is put between UI interaction and database check. This, fixed false positive test pass for not updating database on empty form submission.
f46430b
to
79ed86d
Compare
spec/stimulus/turbo_spec.rb
Outdated
sleep(1) | ||
|
||
# This will ensure we wait enough for the changes to be applied | ||
expect(page).to have_text("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahangarha, this is a very strange assertion!
Please add self-review comments for WTF's like this!
CC: @Judahmeek
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the comment above that line, we need to put some delay before getting assertion for comment count change. Before I fixed it with sleep but that is not efficient. This assertion makes it more efficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect page to display the new comment in the right format block (not in the input field).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already had a test to ensure the new comment is displayed on the page. This test was to ensure the comment was added to the database.
Now, we combine them into one test with multiple assertions.
@@ -52,27 +52,36 @@ | |||
fill_in author_field, with: comment.author | |||
fill_in text_field, with: comment.text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make sure the text is some unique value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this test runs, we have a fresh database.
spec/stimulus/turbo_spec.rb
Outdated
sleep(1) | ||
|
||
# This will ensure we wait enough for the changes to be applied | ||
expect(page).to have_text("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect page to display the new comment in the right format block (not in the input field).
spec/stimulus/turbo_spec.rb
Outdated
sleep(1) | ||
|
||
# This will ensure we wait enough for the changes to be applied | ||
expect(page).to have_text("") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ahangarha see my changes. |
The changes in
rescript_spec.rb
andturbo_spec.rb
improve our test accuracy.The changes inspec/system/*
fix the test failure when these tests run before others (leading to their failure).I am not sure why we have tests in thesystem
directory that should have the type offeature
, but it fixes our issues. I need to study deeper to understand what is happening under the hood.Should I move thesefeature
tests into thefeature
directory, or we can keep them this way for now?This change is