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.

Sign up for GitHub

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

Jump to bottom

Fix flaky test issue with export tests #2080

Merged
merged 3 commits into from
May 22, 2023
Merged

Fix flaky test issue with export tests #2080

merged 3 commits into from
May 22, 2023

Conversation

tonylee80
Copy link
Contributor

@tonylee80 tonylee80 commented May 19, 2023 •
edited
Loading

Address flaky export test issue, where duplicate resources are created.
https://github.com/medplum/medplum/actions/runs/5026350802/jobs/9014520800

Remove redundant flaky tests. There are other tests from src/fhir/operations/export.test.ts and src/fhir/operations/groupexport.test.ts that provides overlapping test coverage.

Test Coverage Before changes:

tonylee@tony-mbp server % npm t expo -- --coverage | grep export               
PASS src/fhir/operations/export.test.ts
PASS src/fhir/operations/groupexport.test.ts
  export.ts                |   97.29 |    94.44 |   85.71 |   97.22 | 34                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        
  groupexport.ts           |   88.88 |    66.66 |      75 |   88.88 | 39,55,68                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  
  bulkexporter.ts          |   94.87 |       75 |     100 |   94.87 | 79,94                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     

Test Suites: 2 passed, 2 total
Tests:       6 passed, 6 total
Snapshots:   0 total
Time:        4.324 s
Ran all test suites matching /expo/i.

Test coverage after changes:

tonylee@tony-mbp server % npm t expo -- --coverage | grep export
PASS src/fhir/operations/export.test.ts
PASS src/fhir/operations/groupexport.test.ts
  export.ts                |   97.29 |    88.88 |   85.71 |   97.22 | 34                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                        
  groupexport.ts           |   88.88 |    66.66 |      75 |   88.88 | 39,55,68                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                  
  bulkexporter.ts          |   94.87 |       75 |     100 |   94.87 | 79,94                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     

Test Suites: 2 passed, 2 total
Tests:       4 passed, 4 total
Snapshots:   0 total
Time:        3.426 s
Ran all test suites matching /expo/i.

@vercel
Copy link

vercel bot commented May 19, 2023 •
edited
Loading

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) May 21, 2023 5:47am
medplum-www ⬜️ Ignored (Inspect) May 21, 2023 5:47am

@tonylee80 tonylee80 force-pushed the tony-export-flaky-test branch 3 times, most recently from 363df71 to 86eae10 Compare May 19, 2023 22:17
@tonylee80 tonylee80 changed the title Fix export flaky test issue with _since params Fix export flaky test issue with export tests May 19, 2023
tonylee80
tonylee80 commented May 19, 2023
View reviewed changes
packages/server/src/fhir/operations/export.test.ts Outdated
const resourceJSON = dataRes.text.trim().split('\n');
expect(resourceJSON).toHaveLength(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we are noticing duplicate resources being created in export tests, so proactively removing the assertion on number of observation results.

@tonylee80 tonylee80 marked this pull request as ready for review May 19, 2023 22:24
@tonylee80 tonylee80 requested a review from a team as a code owner May 19, 2023 22:24
@tonylee80 tonylee80 added this to the May 31, 2023 milestone May 19, 2023
@tonylee80 tonylee80 added the fhir-datastore Related to the FHIR datastore, includes API and FHIR operations label May 19, 2023
@tonylee80 tonylee80 force-pushed the tony-export-flaky-test branch 5 times, most recently from 6b554b2 to e618bb9 Compare May 20, 2023 03:14
@coveralls
Copy link

Coverage Status

Coverage: 94.311%. Remained the same when pulling 60b47d4 on tony-export-flaky-test into 4d91355 on main.

@tonylee80 tonylee80 changed the title Fix export flaky test issue with export tests Fix flaky test issue with export tests May 20, 2023
@tonylee80 tonylee80 marked this pull request as draft May 21, 2023 04:16
@tonylee80 tonylee80 force-pushed the tony-export-flaky-test branch 3 times, most recently from bc59b28 to 770cfcc Compare May 21, 2023 05:43
tonylee80
tonylee80 commented May 21, 2023
View reviewed changes
packages/server/src/fhir/operations/export.test.ts
@@ -99,211 +99,6 @@ describe('System export', () => {
expect(JSON.parse(resourceJSON[0])?.subject?.reference).toEqual(`Patient/${res1.body.id}`);
});

test('Parameters', 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.

These tests using _since parameter is redundant and doesn't provide additional test coverage.
since parameter is also tested in

medplum/packages/server/src/fhir/operations/groupexport.test.ts

Line 128 in e8bff43

test('Since filter', async () => {

@sonarcloud
Copy link

sonarcloud bot commented May 21, 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 May 21, 2023 08:08
tonylee80
tonylee80 commented May 21, 2023
View reviewed changes
packages/server/src/fhir/operations/export.test.ts
expect(JSON.parse(resourceJSON[0])?.code?.text).toEqual('test2');
});

test('Multiple Resources by Resource Type', async () => {
Copy link
Contributor Author

@tonylee80 tonylee80 May 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This test coverage is redundant, there was a later test below that provided the same coverage. Deleting the test since it was also flaky before #2022 . https://github.com/medplum/medplum/pull/2080/files#diff-454070ab471e4a54926cd48d6d4e8e030e3158b06f16797fd9774ed037d467e9L307

codyebberson
codyebberson approved these changes May 22, 2023
View reviewed changes
Copy link
Member

@codyebberson codyebberson left a comment

Choose a reason for hiding this comment

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

Oo, nice. Even better. Thanks @tonylee80 !

@tonylee80 tonylee80 merged commit 3c9586b into main May 22, 2023
@codyebberson codyebberson deleted the tony-export-flaky-test branch May 22, 2023 21:07
@tonylee80 tonylee80 mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codyebberson codyebberson codyebberson approved these changes

Assignees

@tonylee80 tonylee80

Labels
fhir-datastore Related to the FHIR datastore, includes API and FHIR operations
Projects
Medplum Product Roadmap
Archived in project
Milestone
May 31, 2023
Development

Successfully merging this pull request may close these issues.
3 participants
@tonylee80 @coveralls @codyebberson