(2995) Fix: flaky sign in spec #2267
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mec
force-pushed
the
fix/2995-flakey-otp-sign-in-spec
branch
4 times, most recently
from
275f734
to
4659b01
Compare
mec
force-pushed
the
fix/2995-flakey-otp-sign-in-spec
branch
2 times, most recently
from
059f6ca
to
88164b2
Compare
CristinaRO
reviewed
Dec 1, 2023
There was a problem hiding this comment.
Thank you for tackling this tricky issue! I'm still ruminating on it, mainly on the topics of:
- do we still have an end-to-end feature spec that is not too mocked/stubbed?
- do we want such a spec, given that what is under test is not the third-party gems, but our implementation?
mec
force-pushed
the
fix/2995-flakey-otp-sign-in-spec
branch
from
88164b2
to
2bc1030
Compare
We have seen this spec fail in CI a number of times with what appered to be the OTP sent to Notify no being the same as `otp_at_time_of_login`, here is an example: https://github.com/UKGovernmentBEIS/beis-report-official-development-assistance/actions/runs/7020362563/job/19099874908#step:6:9200 Our hypothesis is that this was caused by some kind of variance in the time between the setting of `otp_at_time_of_login` and the expectation. During this work we tried to understand how we have implemented the 2FA in the application. This work seems to indicate that although we configure the expiry and drift (a window of time to allow for differences in the server and client time) of the token to 5 minutes, the acutal time is 5 minutes and 30 seconds. Regardless, we decide the best we could do is use more strict time control around when the user is signing in and when they enter the token in this feature spec. By using as close to a resolution in seconds (time is never going to be perfect!) we can demonstrate both the expected behaviours: - token is valid up to 5 minutes 29 seconds after creating it - token is invalid from 5 minutes 30 seconds after creating it To be honest, we are not sure at this point if this will prevent the spec flaking, but it is a good start. We will document what we've learnt in a later commit.
In previous work we tightened up the time control in this spec as we were seeing occasional failures. However, in the context of this spec, I don't think we care if the OTP token is correct, only that: - the right token is sent to Notify - that with the correct token, the user is signed in Here we have made some changes to the spec so that ANY token is considered correct, this should decouple any time issues that relate to the token. We have left the time controls in as documentation for the expiring of the token, even though they are not required.
Whilst investigating a flaky spec, we learnt about our implementation so are documnenting what we learnt for the future.
mec
force-pushed
the
fix/2995-flakey-otp-sign-in-spec
branch
from
2bc1030
to
0dafeef
Compare
|
@CristinaRO I've updated the subbed OTP to a string as we discussed. This is ready to have another look. 👍 |
CristinaRO
approved these changes
Dec 11, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There are two quite different changes here:
The first tightens the time control around the user signing in and the spec storing the OTP for comparing later.
The second makes the OTP always correct, decoupling the spec from the OTP entirely.
I've documented what I learnt along wth the changes to the acutal specs.
I found that the OTP seems to last 5 mintutes and 30 seconds, even though it is configured to 5 miniutes. I've added a spec that demonstrates the odd extra 30 seconds and encourage you to have a play with the specs and see what you think and let me know.
I am confident the end result here will stop the flaking, but other folks tastes may vary so I am leaving this draft for now.
Appreciate any other views on this spec flaking, there is an example CI run linked in the commit, but here it is again:
https://github.com/UKGovernmentBEIS/beis-report-official-development-assistance/actions/runs/7020362563/job/19099874908#step:6:9200
https://trello.com/c/AmrnslpW