-
Notifications
You must be signed in to change notification settings - Fork 61
fix: codeflare-operator e2e tests not passing #312
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: codeflare-operator e2e tests not passing #312
Conversation
test/support/utils.go
Outdated
|
||
// Get output directory and create the directory if it doesn't exist | ||
outputDir := t.OutputDir() | ||
err := os.MkdirAll(outputDir, os.ModePerm) |
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.
If we really want to have the parent directory created, it'd be better to encapsulate the logic in the OutputDir()
method. Still the existing assumption is that the parent directory exist, and the output directory is created there.
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 @astefanutti ,
For some reason the /tmp/TestMNISTRayJobMCADRayCluster directory isn't being created before that, so the logs couldn't be saved.
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.
In what scenario did it fail? Is it when you run the test locally?
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.
Yea, I've been running the codeflare-operator locally via openshift
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.
OK, so when run locally, the CODEFLARE_TEST_OUTPUT_DIR
environment variable is supposed to be used. It's somehow designed so as to avoid having the test execution polluting the disk by mistake.
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.
Very Nice :D Thanks @astefanutti these changes have been pushed.
}, | ||
WorkingDir: "/test2", |
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.
Can we find a better name for the volume and the directory, maybe workdir
?
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 @astefanutti , I'll update this :)
}, | ||
WorkingDir: "/test2", |
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.
Can we find a better name for the volume and the directory, maybe workdir
?
c98a5e6
to
8ce11de
Compare
Thanks! /lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: astefanutti 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 |
Issue link
#313
What changes have been made
Using the 'PYTHONUSERBASE' environment variable, to point to the emptyDir volume mount for mnist_pytorch_mcad_job_test and mnist_raycluster_sdk_test
Updated the readme to provide instructions to setting the CODEFLARE_TEST_OUTPUT_DIR when running e2e tests locally.
Verification steps
Follow the end to end test instructions, and they should now pass, and complete the runthrough :)
Checks