-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add arm/arm64 and chakrafull support to runtests.py #5273
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
Add arm/arm64 and chakrafull support to runtests.py #5273
Conversation
boingoing
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.
LGTM
test/runtests.py
Outdated
|
|
||
| # arch: x86, x64 | ||
| arch = 'x86' if args.x86 else ('x64' if args.x64 else None) | ||
| arch = 'x86' if args.x86 else ('x64' if args.x64 else ('arm' if args.arm else ('arm64' if args.arm64 else None))) |
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... seems a bit ridiculous, especially with the two extra architectures. Could we make it into a dictionary lookup or a regular if tree?
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 see your point, this is changed to a series of if statements in the next commit. I don't know of a good way to do this that's not significantly bigger or harder to read, though.
test/runtests.py
Outdated
| binary = os.path.join('Build', build, 'bin', '{}_{}'.format(arch, flavor), binary_name) | ||
| else: | ||
| binary = 'out/{0}/ch'.format(flavor) | ||
| binary = os.path.join('out', '{0}'.format(flavor), binary_name) |
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.
'{0}'.format(flavor) can just be flavor.
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.
Addressed in new changeset.
|
|
||
| # check ch failed | ||
| if exit_code != 0: | ||
| if exit_code != 0 and binary_name_noext == 'ch': |
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.
Why would this be specific to ch?
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 believe that an uncaught exception in jshost causes us to exit with a non-zero error code, but in ch it causes us to exit with a 0 error code. We have some tests where the baseline includes the error message at the end. We may want to normalize this behavior in the future, but that's out-of-scope for this change.
c4ef8a9 to
4849ccb
Compare
… runtests.py Merge pull request #5273 from Penguinwizzard:runtestspy_arm64_full This allows running jshost on the core unit tests easily from the python runner, as well as running core tests on arm and arm64. Note: hold this change, and re-target to release/1.10 when ready.
…ull support to runtests.py Merge pull request #5273 from Penguinwizzard:runtestspy_arm64_full This allows running jshost on the core unit tests easily from the python runner, as well as running core tests on arm and arm64. Note: hold this change, and re-target to release/1.10 when ready.
This allows running jshost on the core unit tests easily from the python runner, as well as running core tests on arm and arm64.
Note: hold this change, and re-target to release/1.10 when ready.