-
-
Notifications
You must be signed in to change notification settings - Fork 812
Create more cypress tests and utilities #8494
Conversation
jryans
left a comment
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.
Seems like a reasonable step forward! 😄 Perhaps we'll later find we want to split these steps up a bit more, but seems good for now.
dbkr
left a comment
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.
Generally looks great - could you also update docs/cypress.md with the state of play?
|
|
||
| // Await Synapse healthcheck | ||
| await new Promise<void>((resolve, reject) => { | ||
| childProcess.execFile("docker", [ |
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.
Why do we need to run this as a curl in the docker container? Couldn't we just poll in javascript?
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.
Oh, I guess you get the retries / timeout for free, and you know there's a curl in the docker container. If I've guessed correctly please comment. :)
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.
Yes correct, we can't use cy.request as it doesn't retry on total connection failure
We could use http but that's horribly unusable, don't want to have to bring in another dep like fetch or axios where it isn't needed
This is a price of leaning into the cypress anti-pattern of standing up servers in tasks
| const username = Cypress._.uniqueId("userId_"); | ||
| const password = Cypress._.uniqueId("password_"); |
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 fun - is this a cypress internal or something and what's it doing?
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.
Its a re-exported lodash https://lodash.com/docs/4.17.15#uniqueId - sequential id generator, nothing fancy
dbkr
left a comment
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.
Thanks!
This change is marked as an internal change (Task), so will not be included in the changelog.