-
Notifications
You must be signed in to change notification settings - Fork 0
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
report java crash from junit.py #56
report java crash from junit.py #56
Conversation
command = kargs.get("command") | ||
app = kargs.get("app") | ||
args = kargs.get("args") | ||
env = kargs.get("env") | ||
cmdloader = kargs.get("cmdloader") | ||
|
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.
cleanups, removed ones were not used
signal.signal(signal.SIGTERM, handle_sigterm) | ||
signal.signal(signal.SIGINT, handle_sigint) | ||
return_code = process.wait() | ||
if 0 != return_code: | ||
print("~ java process exited with exit code %d" % return_code) |
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.
Important bit, logging return code to be picked up by CI
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 minor: "java process exited with code %"?
if os.path.exists(os.path.join(app.path, 'test-result/result.passed')): | ||
print("~ All tests passed") | ||
print("~") | ||
testspassed = True | ||
if os.path.exists(os.path.join(app.path, 'test-result/result.failed')): | ||
print("~ Some tests have failed. See file://%s for results" % test_result) | ||
print("~") | ||
sys.exit(1) |
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 particular files are not generated by play.test.Runner
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.
I'm not sure what this means. Isn't it needed?
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.
I was confused by this code and the fact that neither test-result/result.passed
nor test-result/result.failed
is generated so i decided to clean it up, it doesn't look like these log messages are picked up by CI and there are another log messages about the results of the run from the java code
junit(app, args) | ||
|
||
def junit(app, args): | ||
app.check() | ||
print("~ Running junit") |
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.
is this needed?
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.
Not really, it rather was confirmation of what's executed on CI to tell apart from https://github.com/Nosto/play1/blob/master/framework/pym/play/commands/test.py#L25 , i don't mind cleaning this up
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.
looks good to me 👍🏼
Pull Request Checklist
Helpful things
Fixes
Fixes #xxxx
Purpose
Logs explicitly the return code for java process in order to be able to notice the crashes
Background Context
Issues on the CI most likely due to OOM killer
References
Are there any relevant issues / PRs / mailing lists discussions?