-
Notifications
You must be signed in to change notification settings - Fork 678
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
test(graphql): fix broken tests for chaoscenter/graphql/server
and integrate mockery to generate mocks
#4372
test(graphql): fix broken tests for chaoscenter/graphql/server
and integrate mockery to generate mocks
#4372
Conversation
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
b42bea2
to
4727e92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀 Introducing mockery
seems like a good idea.
@@ -31,19 +31,17 @@ import ( | |||
"go.mongodb.org/mongo-driver/mongo" | |||
) | |||
|
|||
var ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify why we need to move global variables to local variables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@namkyu1999 I explained here in the comment #4285 (comment), I will update the PR description and also add AssertExpectations
in the tests to help avoid similar situation where unmet expectations are causing weird behaviour. Will also address the codacy analysis thanks for your prompt review :)
Could you also check |
Just an update: After adding
I am working on cleaning this up now and ensuring mocks aren't shared between two tests |
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
5df5358
to
669093c
Compare
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
a75db79
to
b4863c2
Compare
@namkyu1999 I am bit lost with codacy checks on the markdown file. It seems to be a conflicting opinion. Could please point me in right direction? Thanks in advance :) Also updated the PR, could you please take another look? |
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
Signed-off-by: smit thakkar <smit.thakkar@deliveryhero.com>
@namkyu1999 Codacy checks seem to have passed; adding three spaces after |
What do you think @Saranya-jena ? |
@smitthakkar96 @namkyu1999 Since this is not a mandatory check, we can ignore this error. Also if you want to contribute on updating this codacy configuration, please feel free to create an issue for the same, we would love to have this contribution. |
Proposed changes
While working on #4370 I noticed that tests are broken on master (example) making it difficult to verify the correctness of my changes. This PR aims to fix the broken tests and, as an added bonus, introduce mockery to make it easy to generate mocks for the interfaces.
Why did moving global mocks to mocks per test fix the flakiness?
Since we had global mocks, some unused mock expectations were leaving side effects, causing other tests to fail when the whole suite was run together. You can read explanation in this or this comment. Also attaching a small example:
Why some mock calls were removed?
As explained in this comment they were unused.
Types of changes
What types of changes does your code introduce to Litmus? Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Dependency
Special notes for your reviewer: