-
Notifications
You must be signed in to change notification settings - Fork 448
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
gh-actions: Extend action to run Frontend Unit tests #1998
gh-actions: Extend action to run Frontend Unit tests #1998
Conversation
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.
@orfeas-k Thanks for creating this!
I left a few comments.
/assign @kimwnasptd
698d600
to
0e90f57
Compare
0e90f57
to
240e641
Compare
While working on this PR, I noticed that all jobs were queued and ran and thus, had to wait for a long time in order to verify if something breaks. After some discussion with @kimwnasptd , we thought it would be preferable if we could eliminate and run the jobs that are specific to each PR's changes. For that, we could utilise https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-excluding-paths and exclude the frontend path in jobs that are not related to it. WDYT? Which workflows do you think we should run when there's a change in the frontend? cc @johnugeorge @andreyvelich @tenzen-y |
I think, for UI related changes, we need to run only https://github.com/kubeflow/katib/blob/master/.github/workflows/katib-ui-e2e-test.yaml and can skip all e2e tests. @tenzen-y Any comments? |
@johnugeorge @tenzen-y in this PR we also changed the .github/workflows/test-node.yaml for running the unit tests, and would extend it in the future to also run the integration tests. Should we include this action as well for running unit/integration tests or should we migrate everything for the UI into the action you mentioned above? |
Extend Frontend Test action to run also KWA frontend unit tests. Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
@kimwnasptd If the integration tests requires images to be built, it is best to be added in https://github.com/kubeflow/katib/blob/master/.github/workflows/katib-ui-e2e-test.yaml as the workflow already does it for you. |
240e641
to
e1170f9
Compare
@johnugeorge
I agree with you. @kimwnasptd |
e1170f9
to
61e3598
Compare
61e3598
to
52528c6
Compare
@kimwnasptd @johnugeorge @tenzen-y I pushed another commit in order to exclude workflows from running when there are changes only in the frontend, just as we discussed here. I'm also removing the |
Prevent the following workflows when a PR contains changes that affect only the frontend: - Charmed Katib - E2E Test with darts-cnn-cifar10 - E2E Test with enas-cnn-cifar10 - E2E Test with mxnet-mnist - E2E Test with pytorch-mnist - E2E Test with simple-pbt - E2E Test with tf-mnist-with-summaries - Go Test - Publish AutoML Algorithm Images - Publish Katib Core Images - Publish Trial Images - Python Test - Shellcheck Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
52528c6
to
0ee57ce
Compare
Thanks @orfeas-k. LGTM from my side. I think we'll only need to re-run one action. /lgtm |
/assign @tenzen-y |
Signed-off-by: Orfeas Kourkakis <orfeas@arrikto.com>
0ee57ce
to
d358963
Compare
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.
@orfeas-k Thanks for updating!
/lgtm
/assign @kubeflow/wg-automl-leads
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: johnugeorge, orfeas-k The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This is a follow up PR to this #1977. Here, we extend the GH action Frontend Test to include a job that runs KWA frontend unit tests as well. This also is needed before this PR #1991 (comment) is merged to ensure that nothing breaks.