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

Retry flaky secure password test #47649

Merged
merged 1 commit into from
Mar 15, 2023

Conversation

fatkodima
Copy link
Member

@fatkodima fatkodima commented Mar 12, 2023

Nicer diff without whitespace changes

Once in a few times when running active record tests, I got an error due to a flaky test -

test "authenticate_by takes the same amount of time regardless of whether record is found" do
# Warm-up (mostly to ensure the DB connection is established)
User.authenticate_by(token: @user.token, password: @user.password)
# Benchmark.realtime returns fractional seconds. Thus, summing over 1000
# iterations is equivalent to averaging over 1000 iterations and then
# multiplying by 1000 to convert to milliseconds.
found_average_time_in_ms = 1000.times.sum do
Benchmark.realtime do
User.authenticate_by(token: @user.token, password: @user.password)
end
end
not_found_average_time_in_ms = 1000.times.sum do
Benchmark.realtime do
User.authenticate_by(token: "wrong", password: @user.password)
end
end

How it looks:

Failure:
SecurePasswordTest#test_authenticate_by_takes_the_same_amount_of_time_regardless_of_whether_record_is_found [/Users/fatkodima/Desktop/oss/rails/activerecord/test/cases/secure_password_test.rb:46]:
Expected |1.6823710007593036 - 2.2806699990760535| (0.5982989983167499) to be <= 0.5.

I do not remember if I saw exactly this test failing on a CI, but was able to find similar ones - https://buildkite.com/rails/rails/builds/93367#01861474-a5b1-4365-aca3-e395042c8a35/1027-1036, https://buildkite.com/rails/rails/builds/93169#0185f7cc-f78b-403c-af98-46f07c6d19c7/1015-1024.

Wdyt about somehow to generalize this helper, so it can be used in other files or even by users in their tests?

@yahonda
Copy link
Member

yahonda commented Mar 12, 2023

Thanks for opening the pull request, as always.
IMO, these tests are not flaky, but it did not satisfy the expected performance/elapsed time.
In this case we should take a look at why it did not satisfy the expected performance first, not retry the failed test case.

@fatkodima
Copy link
Member Author

Thanks for the comment. Looks like flaky to me, because it works most of the time and fails once in a while with not so much of the difference from the expected difference. This test performs 2 loops running sql queries in them and measures the time. These queries can take a slightly different amount of time on each attempt because of the parallel activity on the system, or, for example, postgres decides to do some cleanup at this time etc.

@byroot
Copy link
Member

byroot commented Mar 13, 2023

Tests that assert performance will be flay by essence, as outside events (e.g. a major GC trigger, or the node the test runs on being very loaded) can easily cause a failure.

So trying it multiple times makes sense to me.

@byroot byroot merged commit 335733b into rails:main Mar 15, 2023
@fatkodima fatkodima deleted the retry-flaky-secure_password-test branch March 15, 2023 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants