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

ID-925 Fix register-user integration test flakiness. #1246

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

Ghost-in-a-Jar
Copy link
Contributor

@Ghost-in-a-Jar Ghost-in-a-Jar commented Nov 20, 2023

https://broadworkbench.atlassian.net/browse/ID-925

The register-user ui integration test is being flakey and I think that the registration flow in orchestration that we changes recently is the cause. In a for yield, futures run concurrently and not necessarily in order unless they depend on results from eachother. So I think what is happening is that sometimes the register request to sam finishes before the rest but when it doesnt we get an error from thurloe and the test fails. By awaiting the sam registration call we make sure it finishes first.

I think this is probably affecting users in production that are trying to register, hard to say how many are actually hitting this.

It is generally good to avoid awaiting futures, but in this case i think it makes sense, register user isnt a high volume endpoint so I am not worried about the marginal performance loss. An alternative is to do future.onSuccess or something but I like the simplicity and straightforward nature of the await here.

Have you read CONTRIBUTING.md lately? If not, do that first.

I, the developer opening this PR, do solemnly pinky swear that:

  • I've followed the instructions if I've made any changes to the API, especially if they're breaking changes
  • I've updated the RC_XXX release ticket with any manual steps required to release this change
  • I've updated the FISMA documentation if I've made any security-related changes, including auth, encryption, or auditing

In all cases:

  • Get two thumbsworth of review and PO signoff if necessary
  • Verify all tests go green
  • Squash and merge; you can delete your branch after this unless it's for a hotfix. In that case, don't delete it!
  • Test this change deployed correctly and works on dev environment after deployment

@Ghost-in-a-Jar Ghost-in-a-Jar changed the title Fix register-user integration test flakiness. ID-925 Fix register-user integration test flakiness. Nov 20, 2023
@Ghost-in-a-Jar Ghost-in-a-Jar force-pushed the fix-register-user-integration-test branch from 33fead1 to edcd0d6 Compare November 20, 2023 17:08
@Ghost-in-a-Jar Ghost-in-a-Jar marked this pull request as ready for review November 20, 2023 17:09
Copy link

codecov bot commented Nov 20, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (95ba657) 69.93% compared to head (6867b64) 69.94%.

Files Patch % Lines
...itute/dsde/firecloud/service/RegisterService.scala 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #1246   +/-   ##
========================================
  Coverage    69.93%   69.94%           
========================================
  Files          101      101           
  Lines         3499     3500    +1     
  Branches       386      371   -15     
========================================
+ Hits          2447     2448    +1     
  Misses        1052     1052           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tlangs
Copy link
Contributor

tlangs commented Nov 20, 2023

Hmmm. I really don't want to Await. I know its probably not a perf concern, but its really not good practice.

Instead, can we use Future.onComplete, or something to force the completion of the Sam call before calling Thurloe?

@Ghost-in-a-Jar Ghost-in-a-Jar force-pushed the fix-register-user-integration-test branch 5 times, most recently from 4cadba8 to a36ea3a Compare November 20, 2023 20:10
@Ghost-in-a-Jar Ghost-in-a-Jar force-pushed the fix-register-user-integration-test branch from a36ea3a to 0943f36 Compare November 20, 2023 20:11
@Ghost-in-a-Jar Ghost-in-a-Jar merged commit d8e5052 into develop Nov 20, 2023
7 checks passed
@Ghost-in-a-Jar Ghost-in-a-Jar deleted the fix-register-user-integration-test branch November 20, 2023 21:54
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