-
Notifications
You must be signed in to change notification settings - Fork 442
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: Resolve errors in e2e tests for cypress in Katib UI #2384
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
4fc9949
to
620e7b8
Compare
This PR relies on a commit in my forked repository: kubeflow/kubeflow@master...tariq-hasan:kubeflow:fix-katib-ui-tests. The commit needs to be merged into the Also the addition of the |
The e2e tests involving Cypress have passed: https://github.com/kubeflow/katib/actions/runs/9935880934?pr=2384. There is a failed test for Katib UI: https://github.com/kubeflow/katib/actions/runs/9935880964/job/27442941694?pr=2384. This is coming from the I am investigating the issue and will have it fixed. |
Thank you so much for investigating this and for this fix @tariq-hasan! |
Sounds good. I'll try to fix the failed Katib UI test so that I can identify if any other code changes are required for Failed Katib UI test: https://github.com/kubeflow/katib/actions/runs/9935880964/job/27442941694?pr=2384 |
d0d4b5f
to
734af5d
Compare
/rerun-workflow "Publish Katib Core Images" |
/rerun-all |
@tariq-hasan @tenzen-y It looks like |
Thank you for the clarification. I did some further investigation on the Publish Katib Core Images workflow. I found that the workflow fails within the final stage of the Dockerfile due to the results coming from If I replace That said, As I will try to do some more investigation to determine why the build is failing and find an appropriate fix. |
Thank you for this deep investigation @tariq-hasan! @kimwnasptd @elenzio9 @orfeas-k @kubeflow/wg-data-leads @ederign @alexcreasy |
@tariq-hasan @andreyvelich I think the answer is already highlighted in that previous commnet
If the build succeeds with |
What changes do we need to make for Kubeflow UI library ? |
@andreyvelich I believe what @orfeas-k meant is that @tariq-hasan generated package-lock.json file in the PR needs to match the node (and npm) version of the node where the 'Publish Katib Core Images' is being executed. @tariq-hasan, could you please double-check that? Also, I'm new to the project, but is there a reason why we are upgrading to node 14 instead of node 16 like kubeflow/kubeflow is using? |
I think, the common Kubeflow frontend component that we use doesn't use Node 16 today: https://github.com/kubeflow/kubeflow/blob/master/components/crud-web-apps/common/frontend/kubeflow-common-lib/package.json#L62 @tariq-hasan Did you check if cypress tests can be succeeded on Node 16 ? If yes, we should just update it. |
I am upgrading That said, the following components are still using node 12: I will also try to reproduce the build from a Dockerfile on my local to ensure that the |
db5c4f1
to
c660b3c
Compare
/rerun-all |
1 similar comment
/rerun-all |
Using However, upgrades of both Please let me know if there are any feedback. I will then create a PR to move |
Yes, @tariq-hasan please create PR into |
Got it. As part of the PR, I will upgrade the following components from version 12 to version 16 to ensure compatibility with |
Thank you for the great investigation! |
@tariq-hasan Any success with submitting PR into |
I apologize for the delay. I've updated the configuration for |
Thank you @tariq-hasan, appreciate your time for it! |
4954f32
to
6fbac8c
Compare
6fbac8c
to
7f2d7db
Compare
Signed-off-by: tariq-hasan <[email protected]>
7f2d7db
to
5fae1d4
Compare
/rerun-all |
I have created a PR for This PR upgrades the node version from 12 to 16 for |
What this PR does / why we need it: This PR fixed the errors in e2e tests for cypress.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #2381
Checklist: