-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix(cts): split requests and e2e tests #3339
Conversation
✔️ Code generated!
|
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.
could you also add this condition in order to exclude them from forks? this way contributions can run and be merged
if: ${{ !github.event.pull_request.head.repo.fork && github.event.number }} |
yes good idea, I will make all the changes to the CI in the next PR, this is just to split them and generate all the files |
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 did not reviewed the mustache changes, in CI we trust
this is great!!
|
||
// We only store tests of clients that ran during this job, the rest stay as is | ||
let testsToStore = matrix[language].toRun | ||
.map((client) => { | ||
const clientName = createClientName(client, language); | ||
const extension = getTestExtension(language); | ||
|
||
return `${testsOutputBase}/client/${clientName}${extension} ${testsOutputBase}/requests/${clientName}${extension}`; | ||
return `${testsOutputBase}/client/${clientName}${extension} ${testsOutputBase}/requests/${clientName}${extension} ${testsOutputBase}/requests_e2e/${clientName}${extension}`; |
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.
maybe names should just be unit and e2e instead or requests
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.
thats a big change, its the good time to do it, but I feel like both client
and request
tests are unit tests, maybe I can just rename request_e2e
to responses
or e2e
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 was more to fit the usual naming of tests with unit, integration and e2e, but indeed both are unit tests
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.
Wonderful execution
🧭 What and Why
🎟 JIRA Ticket: DI-2573
Split e2e to be able to retry them, or exclude them when running the test suite offline.