Skip to content

LG-11624 idv rate limiter bug#9921

Merged
theabrad merged 11 commits intomainfrom
abrad-lg-11624-idv-rate-limiter-bug
Jan 17, 2024
Merged

LG-11624 idv rate limiter bug#9921
theabrad merged 11 commits intomainfrom
abrad-lg-11624-idv-rate-limiter-bug

Conversation

@theabrad
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket:
LG-11624

🛠 Summary of changes

Fixed a flaky test that would fail if the 1 second difference between redis and the test happened. Also changed the assertion for the user to be not rate limited rather than be limited.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Ensure verify_info_step_spec.rb passes.

there is a disconnect with redis and the rate limiter causing the tests
to be flaky if you use a 1 second difference.

I also changed the verify_info_step spec to check if the user is not
rate limited since it would have expired.
changelog: Internal, IdV Tests, Fix flaky verify info spec test
@theabrad theabrad requested a review from a team January 12, 2024 21:35
complete_verify_step

expect(page).to have_current_path(idv_phone_path)
expect(RateLimiter.new(user: user, rate_limit_type: :idv_resolution)).to be_limited
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused why we expected it to be limited, if it's outside of the window, it should have been not limited right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. As best we can figure, this has been wrong for a bit; the other changes made the incorrect test usually pass. This started as a flaky-spec (this one) investigation, and it turned out that the occasional failure was the proper behavior. The problem was confined to the spec, and the duplicate calls to the before action were masking it.

Copy link
Contributor

@jmax-gsa jmax-gsa left a comment

Choose a reason for hiding this comment

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

LGTM with the resets.

complete_verify_step

expect(page).to have_current_path(idv_phone_path)
expect(RateLimiter.new(user: user, rate_limit_type: :idv_resolution)).to be_limited
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah. As best we can figure, this has been wrong for a bit; the other changes made the incorrect test usually pass. This started as a flaky-spec (this one) investigation, and it turned out that the occasional failure was the proper behavior. The problem was confined to the spec, and the duplicate calls to the before action were masking it.

@theabrad theabrad merged commit 77350a8 into main Jan 17, 2024
@theabrad theabrad deleted the abrad-lg-11624-idv-rate-limiter-bug branch January 17, 2024 18:02
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.

3 participants