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

Non deterministic test failure #1042

Merged
merged 4 commits into from
Feb 10, 2023
Merged

Non deterministic test failure #1042

merged 4 commits into from
Feb 10, 2023

Conversation

kb-0311
Copy link
Contributor

@kb-0311 kb-0311 commented Feb 9, 2023

What kind of change does this PR introduce?

bugFix
Issue Number:

Fixes #1010

Did you add tests for your changes?
Yes

Snapshots/Videos:

If relevant, did you update the documentation?

Summary

Does this PR introduce a breaking change?
Probably not

Other information

Have you read the contributing guide?
Yes

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 9, 2023

This PR does require some extra review and second opinion on this approach,

Credits for this solution -> vitest-dev/vitest#1692 (comment)

Copy link
Contributor

@palisadoes palisadoes left a comment

Choose a reason for hiding this comment

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

@kb-0311 Please also fix the test that failed to cause this GitHub Action failure.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 10, 2023

@palisadoes there is not a specific test that fails to throw this unhandled error. If any one test is failed then the uploadImage.ts file gets untracked and throws that error. i ha ve mentioned other details here.

#1010 (comment)

@palisadoes
Copy link
Contributor

Please explain some more.

The same suite of tests always pass in the pull request workflow and then intermittently fails when it is rerun in the push to the repo after the merge.

Why?

This seems to be proving that there is a test that occasionally fails and your test is highlighting the flaw.

Which other test should we be targeting?

Please take a look at all the failed push actions to see whether you can find a culprit.

If you have good reason to think this analysis is incorrect, please state it. This is not an area of strength for me.

@codecov-commenter
Copy link

codecov-commenter commented Feb 10, 2023

Codecov Report

Merging #1042 (d9a9634) into develop (4e2ff67) will increase coverage by 0.42%.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff             @@
##           develop    #1042      +/-   ##
===========================================
+ Coverage    94.55%   94.98%   +0.42%     
===========================================
  Files          154      154              
  Lines         9112     9014      -98     
  Branches       801      762      -39     
===========================================
- Hits          8616     8562      -54     
+ Misses         495      451      -44     
  Partials         1        1              
Impacted Files Coverage Δ
src/resolvers/Mutation/createPost.ts 100.00% <0.00%> (ø)
src/resolvers/Mutation/createOrganization.ts 100.00% <0.00%> (ø)
src/resolvers/Mutation/joinPublicOrganization.ts 100.00% <0.00%> (ø)
src/resolvers/Mutation/registerForEvent.ts 100.00% <0.00%> (+4.31%) ⬆️
src/resolvers/Mutation/logout.ts 100.00% <0.00%> (+4.34%) ⬆️
src/resolvers/Mutation/removeAdmin.ts 100.00% <0.00%> (+4.39%) ⬆️
src/resolvers/Mutation/likePost.ts 100.00% <0.00%> (+4.93%) ⬆️
src/resolvers/Mutation/likeComment.ts 100.00% <0.00%> (+4.93%) ⬆️
src/resolvers/Mutation/rejectMembershipRequest.ts 100.00% <0.00%> (+5.66%) ⬆️
src/resolvers/Mutation/leaveOrganization.ts 100.00% <0.00%> (+6.45%) ⬆️
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 10, 2023

@palisadoes yeah I got what you were saying now, Thankfully @xoldyckk cleared it up.

Fixed that test suite.

Copy link
Contributor

@xoldd xoldd left a comment

Choose a reason for hiding this comment

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

@kb-0311 did you try the changes on your local system? Like intentionally erroring out one of the other tests to see if this issue still persists?

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 10, 2023

@kb-0311 did you try the changes on your local system? Like intentionally erroring out one of the other tests to see if this issue still persists?

@xoldyckk
Yes , I did , forgot to attach the screenshots - >
Here I failed one of the tests purposely but did not get an unhandled exception ->
Screenshot from 2023-02-10 17-55-21

At least these changes do work on my local machine, hopefully, it works on GitHub CI/CD as well.

If it pops up again then there is something else to it.

@xoldd
Copy link
Contributor

xoldd commented Feb 10, 2023

@kb-0311 Do you think the order matters here? Does this occur if uploadImage test runs before other tests and then one of the tests fails or vice versa? Idk if vitest runs tests one by one by going through directories and files sequentially or just one at a time randomly from anywhere.

If it's random then it'd be better to check if the error persists for the following cases:-

random failing test -> uploadImage test runs
uploadImage test runs-> random failing test

You can pick any random test and run it together with uploadImage.spec.ts(i.e., two tests). Like this:-

npx vitest ./tests/utilities/uploadImage.spec.ts ./tests/resolvers/intentionallyFailedTest.spec.ts

Try running it many times.

@kb-0311
Copy link
Contributor Author

kb-0311 commented Feb 10, 2023

@xoldyckk

  1. Running ->
npx vitest ./tests/utilities/uploadImage.spec.ts ./tests/resolvers/intentionallyFailedTest.spec.ts

Screenshot from 2023-02-10 20-12-42
No unhandled error thrown, looks good.
2. Running ->

npx vitest ./tests/resolvers/intentionallyFailedTest.spec.ts ./tests/utilities/uploadImage.spec.ts 

Screenshot from 2023-02-10 20-14-47
Same thing, no unhandled exception thrown, looks good.

@palisadoes palisadoes self-requested a review February 10, 2023 15:09
@palisadoes palisadoes merged commit 88886b9 into PalisadoesFoundation:develop Feb 10, 2023
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.

Fix tests for isAuth.spec.ts and src/server.ts
4 participants