-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Improve the integration test code #20172
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
base: master
Are you sure you want to change the base?
Improve the integration test code #20172
Conversation
/botio integrationtest |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/e4b49f80e27d9af/output.txt |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/15189469c37f312/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/15189469c37f312/output.txt Total script time: 14.54 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/e4b49f80e27d9af/output.txt Total script time: 31.38 mins
|
await page.waitForFunction( | ||
`document.querySelector("[data-annotation-id='25R']").hidden === false` | ||
`document.querySelector('${selector}').hidden === false` |
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.
In cases like this, it would be better to do
`document.querySelector('${selector}').hidden === false` | |
`document.querySelector(${JSON.stringify(selector)}).hidden === false` |
so that JSON.stringify
takes care of properly handling quotes. Otherwise we need to rely on the "implementation detail" that getAnnotationSelector
returns a string containing "
and no '
.
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.
Good point. I agree that relying on the quoting in getAnnotationSelector
is not optimal, but while the JSON.stringify
approach works for the case where we only have a single selector I don't yet see how this can work for cases like document.querySelector('${selector} > :first-child').checked
where the selector is the inner part of a bigger string, as the surrounding string will still need quotes that aren't equal to the inner ones. From the console:
> const selector = '[data-annotation-id="7R"]';
> `document.querySelector(JSON.stringify("${selector} > :first-child")).checked`
'document.querySelector(JSON.stringify("[data-annotation-id="7R"] > :first-child")).checked'
> document.querySelector(JSON.stringify("[data-annotation-id="7R"] > :first-child")).checked'
Uncaught SyntaxError: identifier starts immediately after numeric literal
In this case we'd still need to know that the surrounding quotes need to be single quotes. I'm probably missing something here, but how would we handle such cases with this approach?
35aa389
to
04eb081
Compare
d5e59f0
to
b4367f0
Compare
/botio integrationtest |
From: Bot.io (Linux m4)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/9d7197f7ba2fa30/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_integrationtest from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/eee4754c646368f/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.241.84.105:8877/9d7197f7ba2fa30/output.txt Total script time: 18.54 mins
|
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/eee4754c646368f/output.txt Total script time: 39.69 mins
|
8b139d1
to
8e988d3
Compare
This test called `closeSinglePage` manually at the end of the test, which is inconsistent with all other tests that call `closePages` in an `afterEach` block. This commit fixes the difference for consistency.
…on test code The helper function was used in a number of places, but also a lot of places contained the annotation selector string inline. This commit makes sure that all places use `getAnnotationSelector` consistently to make sure the annotation selector string is only defined in a single place and to improve readability of the test code.
Most places have a newline before/after `before{Each,All}`, `after{Each,All}` and `it` to visually separate the blocks for clarity, but in a handful of places this wasn't done. This commit removes the inconsistencies so that the test code is formatted consistently.
8e988d3
to
6d482be
Compare
The commit messages contain more information about the individual changes.