-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
fix: GraphQL Playground HTML #9058
fix: GraphQL Playground HTML #9058
Conversation
Fixed double quoting issue with "${JSON.stringify()}". Signed-off-by: Tyrese, Full Stack Genius <133923231+Tyrese-FullStackGenius@users.noreply.github.com>
I will reformat the title to use the proper commit message syntax. |
Thanks for opening this pull request! |
Is it possible to add a test for this? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #9058 +/- ##
==========================================
+ Coverage 84.95% 93.78% +8.83%
==========================================
Files 186 186
Lines 14687 14687
==========================================
+ Hits 12477 13774 +1297
+ Misses 2210 913 -1297 ☔ View full report in Codecov by Sentry. |
it's frontend bug, so I think you have to run the server to test. |
Which server are you referring to? |
parse-server:7.0.0 |
This is parse sever. Running it is part of the testing. |
understand, this is server start code:
|
If you are familiar with testing frameworks in nodejs / js it should be easy for you to add. You could just take a look at the existing tests and add one for this bug. Note that you may have exposed your DB access credentials in the comment above. |
where is test code example? |
In /specs |
ok |
@mtrezza I added test code to check render using puppeteer, and tested with that. |
@@ -582,6 +583,26 @@ describe('ParseGraphQLServer', () => { | |||
}); | |||
}); | |||
|
|||
describe("Playground Render", () => { | |||
it("should properly render the playground without JavaScript errors", async () => { |
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.
Wouldn't it be much simpler to check the response from http://localhost:13377/playground
for the correct headers? Not sure what effect initialEndpoint
has and how that could be checked.
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.
Yes, but I think rendering checks are good for the the future because if there are other errors that method can't catch them.
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.
how do you think about it?
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.
What errors specifically are you referring to? It would probably be more effective to have specific tests. A general "does it render a page" is very broad and doesn't check for anything specific.
In this specific bug we want to check for headers. I guess the test executes faster if you just check the headers in the response? Did you compare test run times?
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.
Any JavaScript rendering errors, such as script not found or other errors in the script;
It's using the script online, errors may occur if the online resource is updated.
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.
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.
I've updated eslintrc and error is fixed
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.
No, the test you added was failing, it timed out.
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.
can you show me the full error? I can't see it.
I can't understand mongodb and postgres test why related to my code
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.
If you open one of these failing jobs you can see the error.
Pull Request
Fixed double quoting issue with "${JSON.stringify()}".
Issue
#9057
Closes: #9057
Approach
Remove quotes outside JSON.stringify()