-
Notifications
You must be signed in to change notification settings - Fork 2
tests: add more logs #64
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
Conversation
Signed-off-by: Pablo Garay <[email protected]>
|
/ok to test eb967b5 |
chtruong814
left a comment
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.
Some comments to refactor the code.
| except subprocess.CalledProcessError as e: | ||
| pytest.fail(f"WAN pretrain mock run failed with return code {e.returncode}") | ||
| print("STDOUT:", e.stdout) | ||
| print("STDERR:", e.stderr) |
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 seems like you want to print out the stdout and stderr regardless? If that's the case, better to do that in a finally block and remove lines 95 and 96. That way, whether it fails or succeeds, you print the stdout and stderr
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.
updated
| except subprocess.CalledProcessError as e: | ||
| pytest.fail(f"DiT pretrain mock run failed with return code {e.returncode}") | ||
| print("STDOUT:", e.stdout) | ||
| print("STDERR:", e.stderr) |
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 seems like you want to print out the stdout and stderr regardless? If that's the case, better to do that in a finally block and remove lines 77 and 78. That way, whether it fails or succeeds, you print the stdout and stderr
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.
updated
Signed-off-by: Pablo Garay <[email protected]>
Signed-off-by: Pablo Garay <[email protected]>
|
/ok to test 882194d |
* tests: add more logs Signed-off-by: Pablo Garay <[email protected]> * update Signed-off-by: Pablo Garay <[email protected]> * update2 Signed-off-by: Pablo Garay <[email protected]> --------- Signed-off-by: Pablo Garay <[email protected]>
No description provided.