-
Notifications
You must be signed in to change notification settings - Fork 422
test(wtr): run in multiple browsers and with API_VERSION=66 #5533
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?
Conversation
I'm not convinced this is the right place to do it
FAIL: packages/@lwc/engine-dom/dist/index.js: 24.79KB > 24.79KB RUDE
installing in package break nucleus
/nucleus ingore --reason "PR isn't ready and I want to watch CI" |
❌ Error Parsing CommandUnknown arguments: reason, ingore
|
/nucleus ignore --reason "PR isn't ready and I want to watch CI" |
* test(wtr): change from filename-based config to magic directive * test(wtr): actually use JSON * test(wtr): let spec file set config for whole dir * test(wtr): fix CTE tests
5bf64d8
to
e928017
Compare
hopefully it doesn't break
not working as intended; may fix later
it got deleted oops
browserName: 'safari', | ||
browserVersion: 'latest', | ||
}), | ||
...(options.LEGACY_BROWSERS |
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 somewhat misleading, how much does things change in (2 versions prior to latest?) to call them legacy?
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.
It's a silly name, but it's what we did with the Karma tests.
import { API_VERSION } from '../../../helpers/options'; | ||
|
||
it(`should support call expressions`, () => { | ||
const cteEnabled = API_VERSION >= 66; |
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.
thinking if this should be hoisted to global config or something so it's easier to update this in the future and we don't have to drill down all the way?
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.
CTE was introduced in API version 66; this check will never need to be updated.
// Potential workaround: https://github.com/modernweb-dev/web/issues/2588 | ||
concurrency: 1, | ||
browserLogs: false, | ||
concurrency: options.CI ? 6 : 1, |
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.
curious about 6
, does this follow any known limitation?
Also, any thoughts on decoupling concurrency with CI and making it an ENV variable?
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 copied this from the docs. When run locally, tests cannot be run concurrently, due to the timing issue mentioned in the code comment. I don't know whether or not the limitation exists when run with SauceLabs, so I made this change to find out.
to see if it makes it less flaky
This reverts commit 41ce390.
browserName: 'chrome', | ||
browserVersion: 'latest', | ||
}), | ||
// sauceLabsLauncher({ |
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.
SauceLabs seems to be flaky. So I'm starting with running just one browser, and I'll add more later.
Details
This PR adds a CI run of the tests with
API_VERSION=66
and updates the config to run in chrome, firefox, and safari. It also (hopefully) connects to saucelabs when run in CI. If theLEGACY_BROWSERS
var is set in CI, then it runs slightly old chrome and safari (not that legacy, imo 🤷).Does this pull request introduce a breaking change?
Does this pull request introduce an observable change?
GUS work item