-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Test improvements #15609
base: master
Are you sure you want to change the base?
Test improvements #15609
Conversation
53cb01e
to
20aca07
Compare
These tests were executed using the PR testing grid. |
59e00e0
to
a5adac5
Compare
tests/wdio.grid.firefox.conf.ts
Outdated
const gridUrl = new URL(process.env.GRID_HOST_URL as string); | ||
const protocol = gridUrl.protocol.replace(':', ''); | ||
|
||
export const config = merge(defaultConfig, { |
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.
Do we need a loadash merge? Couldn't we get away with something like:
{
... defaultConfig,
protocol,
...
}
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 don't know. The example of inheriting configurations is using a new dependency deepmerge-ts and by Saul's proposal (if I remember correctly), I changed it to use loadash and to avoid introducing a new dependency.
https://webdriver.io/docs/organizingsuites#inherit-from-main-config-file
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.
This is how and the existing firefox config is implemented.
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.
But do we use the deeply nested override?
I mean what do we want to happen when we override protocol
for example? Do we want the results to have the new protocol value or we want a merged object and have the old props of protocol and also the new props?
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.
Yeah, you are right. I guess, we can get rid of those merges in the grid file. In the firefox one, we need it as we need it to merge the props and overwrite some of value but not all of 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.
I would also ve fine to leave it as it is since on the other place is like that.
Whatever you decide.
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.
Other than the small comment all LGTM!
a5adac5
to
24a7f75
Compare
Done, I force-pushed changing only the 'Adds target for grid ff tests' commit. I changed the newly added conf and the existing grid conf to get rid of the merge function. |
Desktop sharing is a long one.
Avoid accumulating large files and keeping them per test.
Tests that take time (desktopSharing) before they use one of the browsers (the 4th one), by the time we use it backend may have timed out the websocket (60 seconds). Add every 20 second and execute a print to keep it alive.
We require digit input and do not have a custom validation.
6e23ce2
to
6d824a5
Compare
6d824a5
to
89727d3
Compare
Will be working on some PR-testing changes.