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

Nit: Combine App::state and App::userState to fix test races #9618

Merged
merged 10 commits into from
Jun 12, 2024

Conversation

oskirby
Copy link
Collaborator

@oskirby oskirby commented May 29, 2024

Description

Spotted in the wild: a flaky testSubscription.js.

This was hauntingly familliar to a bug I fixed in one of my experimental hackathons in which vpn.authenticateInApp() returns success as soon as we get to the UserAuthenticated state but the model initialization is still waiting on a bunch of tasks to finish. If this happens during the initial test setup, and we quit before those jobs finish - we are left with a settings file that is incomplete (the token is set but no keys are) and all the subsequent test jobs fail to load.

To fix this properly, I think the best approach is to combine the App::state and App::userState states together to eliminate potential race conditions between them, and to add a new userAuthenticated property that returns true once the auth is completed.

Reference

Cherry-picking a bunch of stuff from PR #9584
Flaky test run: testSubscription.js

Checklist

  • My code follows the style guidelines for this project
  • I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
  • I have performed a self review of my own code
  • I have commented my code PARTICULARLY in hard to understand areas
  • I have added thorough tests where needed

@oskirby oskirby changed the title Naomi flaky subscription tests Nit: Maybe fix flaky subscription tests May 29, 2024
@oskirby oskirby force-pushed the naomi-flaky-subscription-tests branch from 6b36f80 to c9a1875 Compare May 29, 2024 15:04
@oskirby
Copy link
Collaborator Author

oskirby commented May 29, 2024

This is confusing having both a State and UserState that both kind of do the same thing - perhaps a bigger refactoring is in order here. Is this a legacy of the attempted split-app suite thing?

@oskirby oskirby force-pushed the naomi-flaky-subscription-tests branch from 6c72ac5 to 3453322 Compare May 30, 2024 19:51
@oskirby oskirby force-pushed the naomi-flaky-subscription-tests branch from 3453322 to eed3aab Compare May 30, 2024 20:22
@oskirby oskirby changed the title Nit: Maybe fix flaky subscription tests Nit: Combine App::state and App::userState to fix potential test races May 30, 2024
@oskirby oskirby changed the title Nit: Combine App::state and App::userState to fix potential test races Nit: Combine App::state and App::userState to fix test races May 30, 2024
@oskirby oskirby marked this pull request as ready for review June 1, 2024 18:27
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

I really, really like this change. Thank you!

One question on a change. Thanks!

src/app.cpp Outdated Show resolved Hide resolved
@oskirby oskirby requested a review from mcleinman June 11, 2024 19:08
Copy link
Collaborator

@mcleinman mcleinman left a comment

Choose a reason for hiding this comment

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

Thanks, looks great!

@oskirby oskirby merged commit 520b4e2 into main Jun 12, 2024
116 checks passed
@oskirby oskirby deleted the naomi-flaky-subscription-tests branch June 12, 2024 18:17
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.

None yet

3 participants