Add a Cypress Test 🌲 #8295
Add a Cypress Test 🌲 #8295
Conversation
Plus cypress plugins to manage synapses in docker containers
dbkr
added
the
T-Task
Codecov Report
@@ Coverage Diff @@
## develop #8295 +/- ##
========================================
Coverage 29.79% 29.79%
========================================
Files 872 872
Lines 50006 50006
Branches 12723 12723
========================================
Hits 14897 14897
Misses 35109 35109 |
also actually give the test a name
also document the chrome setting
and remove debug logging / comment fixes
and temporarily remove contional to test
to be compatible with fs-extra types
also make password more... sensible
Seems to be unnecessary since the signing key is perfectly fine
.github/workflows/layered-build.yaml
Outdated
| @@ -20,4 +20,29 @@ jobs: | |||
| path: element-web/webapp | |||
| # We'll only use this in a triggered job, then we're done with it | |||
| retention-days: 1 | |||
|
|
|||
| cypress: | |||
There was a problem hiding this comment.
not sure this is in the right workflow file...? "layered build" doesn't scream tests, at least.
There was a problem hiding this comment.
ah yes, unfortunately I think it can only run after a job in the same workflow file (apart from the workflow_run job but that runs with too much permission). Meant to rename the workflow file, and have now done so.
| function randB64Bytes(numBytes: number): string { | ||
| return crypto.randomBytes(numBytes).toString("base64").replace(/=*$/, ""); | ||
| } |
There was a problem hiding this comment.
we should already have a random string utility we can use?
There was a problem hiding this comment.
Mmm, true - I guess importing code from the app would be fine, but it feels like it's probably good hygiene to keep them completely separate, especially when the code in question is as simple as it is. Plus this is node-specific and we specifically want base64 encoding for the ed25519 key.
| const tempDir = await fse.mkdtemp(path.join(os.tmpdir(), 'react-sdk-synapsedocker-')); | ||
|
|
||
| // change permissions on the temp directory so the docker container can see its contents | ||
| await fse.chmod(tempDir, 0o777); |
There was a problem hiding this comment.
20% sure this won't work on windows, but also a good chance that nodejs will have stubbed it
There was a problem hiding this comment.
Nodejs says, "on Windows only the write permission can be changed, and the distinction among the permissions of group, owner or others is not implemented." so sounds like it will work to some extent, or at least no-op.
| "-d", | ||
| "-v", `${synCfg.configDir}:/data`, | ||
| "-p", "8008/tcp", | ||
| "matrixdotorg/synapse", |
There was a problem hiding this comment.
we've previously used develop to ensure forwards compatibility with upcoming features, though there's certainly an argument to be had about whether that's a good idea.
There was a problem hiding this comment.
ah yep - let's keep it as-is for now while we have the argument
| console.log(`Starting synapse with config dir ${synCfg.configDir}...`); | ||
|
|
||
| const synapseId = await new Promise<string>((resolve, reject) => { | ||
| childProcess.execFile('docker', [ |
There was a problem hiding this comment.
can we also give this a --name and set --rm so it cleans itself up upon exiting?
There was a problem hiding this comment.
Have given them a name. We collect logs at the end so if we use --rm then we don't have a chance to get the logs out. I have also made it tidy up any remaining synapses after each spec run to try to avoid containers sitting around if people forget to stop them in the tests.
Co-authored-by: Travis Ralston <[email protected]>
Also: * Move the synapseStart / synapseStop functions out to the top level so they can be reused * Add a tsconfig file * Give the containers names
We don't upload it anyway so tell cypress not to so it can not bother encoding them
and fix existing lint errors
and make it pass the type checks, specifically: * Upgrade sinon fake timers to a version that has the right types * Set module resolution * Type check cypress files separately
Probably better to just call it an element web build
| await new Promise<void>((resolve, reject) => { | ||
| childProcess.execFile('docker', [ | ||
| "rm", | ||
| id, | ||
| ], err => { | ||
| if (err) reject(err); | ||
| resolve(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
might be best to put this on a try { } finally { ... }, but not blocking for this PR
* A first, maybe working cypress test Plus cypress plugins to manage synapses in docker containers * Fix yaml * This file is important * try & find where it's put the artifact * Download artifact to a directory * pics or it didn't happen * Add conditional, otherwise no artifacts on failure... * Try increasing timeout also actually give the test a name * Try in chrome * Get docker logs to see why it's failing also document the chrome setting * Try changing mode on homeserver.yaml * debug * More debugging * more file permissions debugging * ARGH * more debug * sigh * Eugh, that's not how arguments work * Add the option to really allow open registration and remove debug logging / comment fixes * failure to yaml * Upload docker logs as artifacts and temporarily remove contional to test * Put the conditional back * Upgrade types in end to end tests to be compatible with fs-extra types * Try reducing timeout a bit also make password more... sensible * Hex is not octal * Remove file mode Seems to be unnecessary since the signing key is perfectly fine * Give the log files extensions * Rename workflow file now it also does tests * Add cypress scripts * copyright headers * Use ? operator Co-authored-by: Travis Ralston <[email protected]> * Use develop synapse image * Tidy up any remaining synapses after each spec run Also: * Move the synapseStart / synapseStop functions out to the top level so they can be reused * Add a tsconfig file * Give the containers names * Don't upload video on test pass We don't upload it anyway so tell cypress not to so it can not bother encoding them * Enable linting on cypress files and fix existing lint errors * Type check cypress files and make it pass the type checks, specifically: * Upgrade sinon fake timers to a version that has the right types * Set module resolution * Type check cypress files separately * Rename workflow file again Probably better to just call it an element web build * Don't plus + characters in container name * Fix yaml * Stream logs to file * Add note to end to end tester to sya what's been ported * Put docker rm in finally block Co-authored-by: Travis Ralston <[email protected]>
| cy.task<SynapseInstance>("synapseStart", "consent").then(result => { | ||
| synapseId = result.synapseId; | ||
| synapsePort = result.port; | ||
| }); |
There was a problem hiding this comment.
Anti-Pattern: Trying to start a web server from within Cypress scripts with cy.exec() or cy.task() .
-- https://docs.cypress.io/guides/references/best-practices#Web-Servers
I assume we've weighed the risks here?
A Cypress clone of the 'register an account' part of the end-to-end tester.
Note that the trickiest end-to-end test to replicate will be the actual sending an encrypted message from one client to another, since Cypress doesn't support running two browsers/tabs at once. I'd currently say the best way to do this would be to test sending and receiving separately with a bot on the other end. Porting this over doesn't need to block us from writing other Cypress tests though.
Another open problem is how to factor tests like this into our coverage.
This change is marked as an internal change (Task), so will not be included in the changelog.
Preview: https://pr8295--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.