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

Gql mock server #1071

Merged
merged 19 commits into from
Nov 20, 2023
Merged

Gql mock server #1071

merged 19 commits into from
Nov 20, 2023

Conversation

daniel-heppner-ibigroup
Copy link
Contributor

@daniel-heppner-ibigroup daniel-heppner-ibigroup commented Nov 17, 2023

Description:
Adds a GraphQL Mock server to the Percy tests.

PR Checklist:

  • Does the code follow accessibility standards (WCAG 2.1 AA Compliant)?
  • Are all languages supported (Internationalization/Localization)?
  • Are appropriate Typescript types implemented?

| Before | After |
|--------|-------|
|
image
|
image
|

Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

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

Looks like good stuff! There are typos here and there, but no major complaints. It would be really nice to print out somewhere the number of calls to of each graphql mock for tracking redundant calls.

package.json Outdated
@@ -18,7 +18,8 @@
"semantic-release": "semantic-release",
"start": "craco start",
"percy-serve": "serve",
"percy-har-express": "har-express"
"percy-har-express": "har-express",
"percy-mock-server": "node ./percy/mock-server.js"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe rename so we know which one is graphql?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add combined to it, but this script runs the express server that hosts GraphQL and REST endpoints.


## Running and debugging the Percy tests
You can run the Percy tests locally with this command:
```OTP_RR_UI_MODE=normal npx percy exec -- npx jest percy/percy.test.js```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is env needed before the env variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works for me. I think because this is a Bash command, it's okay. I am not sure about Windows.

percy/README.md Outdated

You can disable headless mode by setting `headless: false` in the Puppeteer launch settings in `percy.test.js`. We left a line commented out which you can uncomment to achieve this.

You can also debug the tests by creating a JavaScript Debug Terminal in the VSCode run pane, then running the above commands with breakpoints set in the editor.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of "in the VSCode...", say "in IDEs that have that functionality"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might just remove the mention of IDEs entirely. There are probably multiple ways of doing this.

}
}

const mocks = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice-to-have: Is that possible to output somewhere how many times each mock was used? Having previously all the responses in order helped highlight unnecessarily repeated web requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could write something to do that, it isn't a bad idea. I could put some assertions in the code for the number of times we expect each resolver to be called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I can't do this very easily, the assertions won't work in the mocking server since it's run as a subprocess under the Percy tests. For some reason importing the server and running it directly in the Percy test doesn't work very well, it fails part way through.
I tried adding some code that logs the calls to each resolver at the end, but the function doesn't seem to be called.

Open to ideas here.

percy/har-mock-config.yml Outdated Show resolved Hide resolved
percy/har-mock-config.yml Outdated Show resolved Hide resolved
percy/mock-server.js Outdated Show resolved Hide resolved
@binh-dam-ibigroup
Copy link
Collaborator

@daniel-heppner-ibigroup Also can you address the translation checks, please?

@daniel-heppner-ibigroup
Copy link
Contributor Author

I'd like to address the translation problems with Nearby View in the parent PR after this is merged into it. This is just to get the Percy Tests all working.

Open to ideas on how to log or document the GraphQL requests made.

@miles-grant-ibigroup
Copy link
Collaborator

Checkout screenshot 11/12! Something funky is going on there, not sure if it's related to these changes. Overall the result looks quite promising well done. I'm not fussed with graphql logging for now, I think we can deal with it later. Happy to approve this once we confirm what's going on with that itinerary body screenshot

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

Tiny bit of code cleanup too

percy/har-mock-config.yml Outdated Show resolved Hide resolved
percy/har-mock-config.yml Outdated Show resolved Hide resolved
@daniel-heppner-ibigroup
Copy link
Contributor Author

After looking into it, I think the missing headsigns in the screenshot is actually correct, as we don't request it in the GraphQL query. The old HAR server would return the same response no matter what the GraphQL query was. At some point I guess we stopped requesting the trip.tripHeadsign in the plan query, but the HAR server kept returning it since we didn't update the response data. The new GraphQL server is smart, so it doesn't return it, which made the test look broken even though it's now actually more correct: we caught a bug!
cc: @binh-dam-ibigroup

@binh-dam-ibigroup
Copy link
Collaborator

The new GraphQL server is smart, so it doesn't return it, which made the test look broken even though it's now actually more correct: we caught a bug! cc: @binh-dam-ibigroup

Good finding!

Copy link
Collaborator

@miles-grant-ibigroup miles-grant-ibigroup left a comment

Choose a reason for hiding this comment

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

One last config cleanup and then I think we are ready to go! Thanks so much for all your work on this it looks great

Comment on lines 101 to 102
reportIssue:
mailto: [email protected]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should go

@daniel-heppner-ibigroup daniel-heppner-ibigroup merged commit 49fed5e into nearby-view Nov 20, 2023
6 of 7 checks passed
@daniel-heppner-ibigroup daniel-heppner-ibigroup deleted the gql-mock-server branch November 20, 2023 22:21
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.

3 participants