-
Notifications
You must be signed in to change notification settings - Fork 4k
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
ci(root): remove redundant API unit test #6678
Conversation
✅ Deploy Preview for novu-stg-vite-dashboard-poc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 in the right direction.
However, the on-pr workflow only runs on PR, whilst reusable-api-e2e runs on merge comits in next
too. This change will save time on PR checks, but it has the effect of removing API unit tests from being run on merges to next
.
Given that we don't require branches to be up to date with next
before merging, our current strategy is to run all the unit tests on next
to ensure that changes introduced from PRs not up to date with next
are compatible with what is in next
.
In summary, we need to ensure that both Unit and E2E tests run on all commits to next
. One approach would be to extract the unit tests from on-pr
and run them as a reusable-api-unit-tests
PR.
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.
💯
Good call out. The reusable-* actions we have must be more fine-grained and act as discrete building blocks so that we can mix and match for each use case. I've added this overhaul to the infra team roadmap. |
What changed? Why was the change needed?
On PR we are running this workflow: https://github.com/novuhq/novu/blob/next/.github/workflows/on-pr.yml
which already has a step called
test_unit_services
which runs API unit tests.We don't need another 2x runs of API unit tests inside e2e test workflow (2x runs because the e2e runs twice for enterprise and regular API)/