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

Fixes #2306 - remove console.log from export tests due to race conditions #2322

Merged
merged 5 commits into from
Jun 28, 2023

Conversation

tonylee80
Copy link
Contributor

@tonylee80 tonylee80 commented Jun 28, 2023

Using waitForAsyncJob to wait for an async job like Bulk export to complete before the test cases ends.

Related PRs
#2022
#2080
#1959

https://github.com/medplum/medplum/actions/runs/5404805700/jobs/9819491438?pr=2322#step:6:1405
image

@vercel
Copy link

vercel bot commented Jun 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
medplum-storybook ⬜️ Ignored (Inspect) Jun 28, 2023 6:56pm
medplum-www ⬜️ Ignored (Inspect) Jun 28, 2023 6:56pm

@tonylee80 tonylee80 force-pushed the tony-console-logs-in-export-test branch 2 times, most recently from 3600d09 to 2285ba5 Compare June 28, 2023 03:00
@coveralls
Copy link

coveralls commented Jun 28, 2023

Coverage Status

coverage: 94.17% (-0.02%) from 94.187% when pulling 68e8e24 on tony-console-logs-in-export-test into 0d23121 on main.

@tonylee80 tonylee80 force-pushed the tony-console-logs-in-export-test branch from 429502c to 2285ba5 Compare June 28, 2023 03:43
@tonylee80 tonylee80 force-pushed the tony-console-logs-in-export-test branch from 2285ba5 to c42d5ad Compare June 28, 2023 03:43
@tonylee80 tonylee80 changed the title Fixes #2306 - remove tests causing race conditions Fixes #2306 - remove tests causing console.log Jun 28, 2023
@tonylee80 tonylee80 force-pushed the tony-console-logs-in-export-test branch 2 times, most recently from dc5101c to f3d4bf9 Compare June 28, 2023 03:56
@tonylee80 tonylee80 force-pushed the tony-console-logs-in-export-test branch from f3d4bf9 to b7009f8 Compare June 28, 2023 03:56
@tonylee80 tonylee80 changed the title Fixes #2306 - remove tests causing console.log Fixes #2306 - remove console.log from export tests Jun 28, 2023
@tonylee80 tonylee80 marked this pull request as ready for review June 28, 2023 05:10
@tonylee80 tonylee80 requested a review from a team as a code owner June 28, 2023 05:10
@codyebberson
Copy link
Member

Hi @tonylee80 - I don't think this is the right fix for this. The "database not setup" message implies that there is still something running in the background after the test completes. That usually means we need to wait for a promise or a job to complete.

@tonylee80 tonylee80 force-pushed the tony-console-logs-in-export-test branch from cc10fa8 to b371a44 Compare June 28, 2023 18:25
@tonylee80 tonylee80 marked this pull request as draft June 28, 2023 18:32
@tonylee80 tonylee80 self-assigned this Jun 28, 2023
@tonylee80 tonylee80 added the bug Something isn't working label Jun 28, 2023
@tonylee80 tonylee80 added this to the June 30, 2023 milestone Jun 28, 2023
@tonylee80 tonylee80 linked an issue Jun 28, 2023 that may be closed by this pull request
@tonylee80
Copy link
Contributor Author

Hi @tonylee80 - I don't think this is the right fix for this. The "database not setup" message implies that there is still something running in the background after the test completes. That usually means we need to wait for a promise or a job to complete.

That's a really good point. I'll refactor to use waitForAsyncJob that's similarly used in super.test.ts to ensure the bulk job is complete before continuing to other tests.


let resBody: any;
await waitFor(async () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moving to waitForAsyncJob to ensure all jobs are completed in export tests. Introducing to this pattern for maintaining these export tests will no longer introduce race conditions where a test ends, but a job might still be running in the background.

@tonylee80 tonylee80 changed the title Fixes #2306 - remove console.log from export tests Fixes #2306 - remove console.log from export tests due to race conditions Jun 28, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jun 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@tonylee80 tonylee80 marked this pull request as ready for review June 28, 2023 19:21
@tonylee80 tonylee80 merged commit f9bacca into main Jun 28, 2023
@tonylee80 tonylee80 deleted the tony-console-logs-in-export-test branch June 28, 2023 23:52
codyebberson added a commit that referenced this pull request Jun 30, 2023
Update client-cloudwatch-logs manual mock to use aws-sdk-client-mock (#2342)
Log resource for validator latency warning (#2338)
Minor tweaks to aws init tool and docs (#2340)
Allow contentReference elements to override cardinality rules (#2339)
Fixes #2315 - allow bot success without return value (#2336)
Fix app env vars in publish job (#2335)
Fixes #2310 - CLI rest with --fhir-url-path (#2313)
Unify all of the sleep helper functions (#2331)
Increase Fargate health check grace period (#2330)
Remove PWA and service worker (#2329)
Updating subscription documentation to reflect behavior when resources are deleted (#2328)
Add app to publish.sh (#2326)
Fixes #2306 - remove console.log from export tests due to race conditions (#2322)
Fixed landing page for storybook (#2323)
Bring back sourcemaps (#2309)
Split custom Medplum search parameter definitions into separate file (#2178)
Poll status follow Location header (#2307)
Copy .env.defaults to .env if not exists (#2308)
Allow extra URL path content with Bot execute (#2305)
Fixes #2182- All Patient Export (#2293)
Strict validation of property cardinality (#2300)
Request from ACN (#2299)
Fixed CLI shebang (#2301)
ESBuild (#2298)
Add logs and parity tests for new validator (#2291)
Fixes #2277 - Updated access policies docs (#2292)
Add non-null requirements to values in GraphQL array types (#2290)
Validating Fixed and Pattern Values (#2288)
Fixes #2195 - updated cdk init docs (#2289)
codyebberson added a commit that referenced this pull request Jul 1, 2023
Update client-cloudwatch-logs manual mock to use aws-sdk-client-mock (#2342)
Log resource for validator latency warning (#2338)
Minor tweaks to aws init tool and docs (#2340)
Allow contentReference elements to override cardinality rules (#2339)
Fixes #2315 - allow bot success without return value (#2336)
Fix app env vars in publish job (#2335)
Fixes #2310 - CLI rest with --fhir-url-path (#2313)
Unify all of the sleep helper functions (#2331)
Increase Fargate health check grace period (#2330)
Remove PWA and service worker (#2329)
Updating subscription documentation to reflect behavior when resources are deleted (#2328)
Add app to publish.sh (#2326)
Fixes #2306 - remove console.log from export tests due to race conditions (#2322)
Fixed landing page for storybook (#2323)
Bring back sourcemaps (#2309)
Split custom Medplum search parameter definitions into separate file (#2178)
Poll status follow Location header (#2307)
Copy .env.defaults to .env if not exists (#2308)
Allow extra URL path content with Bot execute (#2305)
Fixes #2182- All Patient Export (#2293)
Strict validation of property cardinality (#2300)
Request from ACN (#2299)
Fixed CLI shebang (#2301)
ESBuild (#2298)
Add logs and parity tests for new validator (#2291)
Fixes #2277 - Updated access policies docs (#2292)
Add non-null requirements to values in GraphQL array types (#2290)
Validating Fixed and Pattern Values (#2288)
Fixes #2195 - updated cdk init docs (#2289)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Console logs in bulk export test
3 participants