-
Notifications
You must be signed in to change notification settings - Fork 659
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
Move to using interface for better mocking to graphql server #3937
Move to using interface for better mocking to graphql server #3937
Conversation
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
… variable Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
I fixed all codes using the interface. So no more global variable |
…d-interface-to-cluster Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
A is current Changes, and B is my proposals. For example in the chaoshub package, As of now A, When I make a unit test in the ChaosHubService(service layer), I cannot create ChaosHubOperator Mock(repository layer) because ChaosHubOperator is not an interface but a struct. So, For better testing, I propose B. I want to know your opinion. @imrajdas @gdsoumya @amityt @S-ayanide |
@namkyu1999 currently the A approach needs you to mock the dboperator instead of the service specific operator so you mock every db method that exists. Imo that should work fine if we can create a generic in-memory mock impl of the db but if that's too complex we can go with B but imo we should avoid going with B if we can as the service specific operator might still have some business logic that we need to test. Also if we go with B then there's no point in having a db interface in the first place as we can just always mock the service specific operator. |
@gdsoumya Thanks for your reply. I agree with your opinion. So No more architectural changes here. And I will make the unit test code in the ChaosHub package this week and raise a different PR. |
Here's my sample unit test code PR |
…d-interface-to-cluster Signed-off-by: namkyu1999 <lak9348@konkuk.ac.kr>
Hi @namkyu1999, can we update the following terminologies in all the logs : |
@Saranya-jena, |
You can only change the logs as I am making the changes in the codebase(currently in a separate branch). Yes you can raise a separate PR for this, posted here to track this. |
Proposed changes
Following up on the last PR, I've converted all packages to the interface pattern for better mocking in graphql server. I made this PR based on this layered architecture.
Issue No. #3931
Prev PR. #3933
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: @S-ayanide @amityt