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

Fix dbAuth test flakiness #4735

Closed
wants to merge 1 commit into from

Conversation

Tobbe
Copy link
Member

@Tobbe Tobbe commented Mar 12, 2022

I noticed we had a flaky test in dbAuth because it's comparing timestamps that aren't guaranteed to be identical.

Here's an example of when it was failing https://github.com/redwoodjs/redwood/runs/5524397642?check_suite_focus=true

image

Notice how one is at 38 seconds and the other at 39 seconds.

The test now allows up to (including) 1 second diff. I doubt it's ever going to be more than a few ms, but 1000 ms is the lowest resolution we get on the timestamps.

@Tobbe Tobbe added the release:fix This PR is a fix label Mar 12, 2022
@Tobbe Tobbe requested a review from cannikin March 12, 2022 23:41
@Tobbe Tobbe self-assigned this Mar 12, 2022
@cannikin
Copy link
Member

Huh, amazing I never saw that locally, I ran that test suite hundreds of times!

Really I should have memoized the time in _futureExpiresDate() so that it's only set the first time you call it, then just returns that same value on future calls. So when the test calls it it'll be exactly the same.

I propose just setting this._futureExpiresDate in the constructor along with the other default values, so that any time that value is accessed via an instance (like in the tests) it'll be the same value. See here: 81f3120 I'll make that a PR if you agree.

That's really what I should have done to begin with, and also no more worries about the test timing and delta and junk!

@Tobbe
Copy link
Member Author

Tobbe commented Mar 13, 2022

I propose just setting this._futureExpiresDate in the constructor along with the other default values

How often is the DbAuthHandler (re-)created?
I'm asking because now the timeout is going to be set once and never updated. If we hold on to the DbAuthHandler that's not going to work. But if it's recreated on each request or something like that, it's fine to set it in the constructor.

@cannikin
Copy link
Member

An instance of DbAuthHandler is created in the auth.js function, so any time that function is called it creates a new instance from scratch: https://github.com/redwoodjs/redwood/blob/main/packages/cli/src/commands/setup/auth/templates/dbAuth.function.ts.template#L123

@Tobbe
Copy link
Member Author

Tobbe commented Mar 13, 2022

@cannikin Great. Sounds like a good solution then. I'll close this PR and let you open a new one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:fix This PR is a fix
Projects
No open projects
Status: Archived
Development

Successfully merging this pull request may close these issues.

2 participants