Use selenium-standalone-service to automatically start and stop Selenium when running the test suite. #4906
Conversation
…ium when running the test suite.
package.json
Outdated
| "dependencies": { | ||
| "jsdom": "15.2.1" | ||
| "jsdom": "15.2.1", | ||
| "typescript": "^4.3.2" |
There was a problem hiding this comment.
Did this make it in by accident? or is typescript a required dependency here?
There was a problem hiding this comment.
It was added automatically by the tooling; the dependencies tab on the package's page on npm doesn't indicate typescript is a dependency, but I'm not sure how I'd otherwise tell?
There was a problem hiding this comment.
Yeah I'm curious how that was added. Try removing it and re-running npm install and seeing if everything works fine without it.
There was a problem hiding this comment.
I tried removing it, npm install was happy, but re-running the tests added it back.
There was a problem hiding this comment.
I suspect this is related to npm test regenerating the TypeScript typing files in typings/.
I'm unclear on whether rebuilding these during test is desirable or not, but in any case if typescript is needed it ought to be installed in the devDependencies section rather than dependencies and—and probably permanently, so that we can be alerted to vulnerabilities, rather than being added ad-hoc by the build/test scripts.
There was a problem hiding this comment.
Yeah, moving it to devDependencies seems to do the trick - it doesn't get added back to dependencies that way. Updated accordingly!
There was a problem hiding this comment.
Oh yeah that makes sense @cpcallen.
Re-building is important as it tells you whether there is a problem with the closure to typescript conversion. Adding it to devDependencies makes sense to me given that requirement.
There was a problem hiding this comment.
It would probably be good to find the code that was previously adding it, and remove it.
| var options = { | ||
| capabilities: { | ||
| browserName: 'firefox' | ||
| browserName: 'chrome', |
There was a problem hiding this comment.
Update the comments for this function to not say that it's using firefox.
Looking forward, should this entire file/set of functions move into gulp scripts? Is there any reason we should keep it as a separate thing? It looks kind of like it went manual testing -> bash script -> node script.
There was a problem hiding this comment.
Done, and perhaps? I'm not familiar enough with the tooling to have a useful opinion, but simplifying/using the standard approach SGTM.
Remove v12 and add v16, per https://nodejs.org/en/about/releases/
Working on fixing RaspberryPiFoundation#2114.
- Do the (hopefully now) correct test to check we are on macOS. - Fix brew command-line syntax for installing google-chrome.
Chrome comes pre-installed on GitHub hosted macOS runners, and while `Xvfb` does not seem to be installed neither does it seem to be needed.
I have been unable to resolve the mocha test issues, so leave macOS disabled for the moment.
…ium when running the test suite.
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
|
@googlebot I consent. |
The calls to `npm run test:prepare` in `tests/scripts/setup_*_env.sh` should have been removed along with that npm script in PR RaspberryPiFoundation#4906.
The calls to `npm run test:prepare` in `tests/scripts/setup_*_env.sh` should have been removed along with that npm script in PR RaspberryPiFoundation#4906.
The calls to `npm run test:prepare` in `tests/scripts/setup_*_env.sh` should have been removed along with that npm script in PR RaspberryPiFoundation#4906.
The calls to `npm run test:prepare` in `tests/scripts/setup_*_env.sh` should have been removed along with that npm script in PR RaspberryPiFoundation#4906.
* chore(tests): Enable testing on node.js v18.x on GitHub CI * chore(tests): Remove outdated calls to test:prepare npm script The calls to `npm run test:prepare` in `tests/scripts/setup_*_env.sh` should have been removed along with that npm script in PR #4906. * chore(deps): Fix alphabetisation in package.json
4 participants
This change also migrates the generator tests to run in Chrome instead of Firefox.
The basics
The details
Proposed Changes
This PR uses the selenium-standalone-service module to automatically start and stop Selenium when running test cases, instead of the old commands in package.json to coordinate doing so. It also uses Chrome to run the generator tests (consistent with the Mocha tests) instead of Firefox.
Behavior Before Change
npm run testwould run the test suite, but hang and never terminate since it was blocking on a child Selenium process.Behavior After Change
npm run testruns the test suite and terminates when it is done.Reason for Changes
The test suite had to be manually stopped with ^C.
Test Coverage
Change is to the test suite itself; ran the test suite and confirmed that all tests pass.