-
Notifications
You must be signed in to change notification settings - Fork 6
#17 makes graphql conform to default Magento and creates tests #66
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
Conversation
@toonvd what was the logic behind this again? |
@toonvd this test already looks very good. There are a couple of cases that still need to be tested:
|
@ThijsFeryn I added more coverage. As requested. I numbered the server reqs also because when the tests get larger I tend to lose count. Any ideas on other things that might help? Do know that other people aside from us need to write tests to get their PRs passed. We need to make this as bearable as possible. |
@toonvd It's perfectly fine to have multiple VTC files to split up the logic |
Check, I think it's fine like this for this test. If you agree please approve and merge. |
@toonvd I added some more test cases to check for hits after misses. I also added some header expectations at the server level to catch potential issues quicker. I'll approve the PR, but please verify my changes and merge the PR if you agree with them. |
This: