xplat: enhance test/runtests.py to use rlexe.xml #983
Conversation
Use `argparse` module to handle command line arguments. (Now this also works on Windows -- needs **python 2.7**.) Read `rlexe.xml` to load test metadata and honor `compile-flags`. Run all loaded tests and print summary at the end. Note this is only one step towards current rl test harness. There are lots of test harness features not implemented yet.
test/runtests.py
Outdated
| js_file = os.path.join(folder, test['files']) | ||
| js_output = "" | ||
|
|
||
| cmd = [x for x in [binary, |
There was a problem hiding this comment.
Not too familiar with this syntax- can you add a comment on whats going on here?
There was a problem hiding this comment.
The outer brackets define a list comprehension scope, x is the loop body and the loop control statement follows it on the same line. It's supposed to read like natural language. The loop puts an element into the list for every value of x that passes the test. The inner brackets are just a list of values.
There was a problem hiding this comment.
I think you could rewrite this with the same semantics as:
cmd = []
for x in [ ... ]:
if x != None:
cmd.append(x)It's not too bad written that way, but the long list explicitly in the for loop would be difficult to read, and it makes sense / is more pythonic to do the list comprehension.
There was a problem hiding this comment.
Thanks for the suggestion! I'll try simplify this -- actually only "compile-flags" could be empty and if so can't be passed to Popen.
|
LGTM- couple comments and couple meta comments:
|
test/runtests.py
Outdated
| parser = argparse.ArgumentParser( | ||
| description='ChakraCore *nix Test Script', | ||
| formatter_class=argparse.RawDescriptionHelpFormatter, | ||
| epilog=textwrap.dedent('''\ |
There was a problem hiding this comment.
I think you could just define the text as a script-level constant instead of defining it inline at this indentation level and taking a dependency on textwrap.
Side note: I'd prefer one package per import line.
There was a problem hiding this comment.
Will do. Copied this from python docs example.
|
Echo @digitalinfinity's concerns. Besides those and my comments, LGTM |
test/runtests.py
Outdated
| else: | ||
| test_dirs[0] = sys.argv[1] | ||
| pass_count = 0 | ||
| fail_count = 0 |
There was a problem hiding this comment.
How about defining a class object for test results and keeping all the test related global information inside? The global variables are usually tend to mix-up at some point.
Will add some and continue to enhance it. Sent this out to unblock "test/Basics" tests first. |
| js_file] if x != None] | ||
| p = SP.Popen(cmd, stdout=SP.PIPE, stderr=SP.STDOUT) | ||
| js_output = p.communicate()[0].replace('\r','') | ||
| exit_code = p.wait() |
There was a problem hiding this comment.
So this test runner runs the tests in serial right now if I am understanding this correctly, is that right?
Plans to to make it parallel?
Also timeout functionality will be desirable if there is a plan to get this in continuous integration at some point.
There was a problem hiding this comment.
Yes, we will make it parallel. Timeout is needed. Currently this is still quite rudimental.
Address review comments. Add TestResult class to record results. Changed `print` calls to try to support python 3.
| type_flavor = {'chk':'debug', 'test':'test', 'fre':'release'} | ||
| flavor = 'debug' if args.debug else ('test' if args.test else None) | ||
| if flavor == None: | ||
| flavor = type_flavor[os.environ.get('_BuildType', 'fre')] |
There was a problem hiding this comment.
If this is hardcoded, why have the map lookup?
There was a problem hiding this comment.
If this is hardcoded, why have the map lookup?
It is not hardcoded. It reads env variable _BuildType, and default to fre if no env variable.
There was a problem hiding this comment.
Oh, thanks for clarifying.
|
Looks like you're delaying the exclude tags work to a future PR? Sounds good to me, just want to make sure that was intentional. |
Yes, future work. This one doesn't even have "print pass skip baseline file" support which we are using everywhere. |
|
@digitalinfinity @dilijev @obastemur @ianwjhalliday Thanks for review and suggestions! |
Merge pull request #983 from jianchun:pyrl Use `argparse` module to handle command line arguments. (Now this also works on Windows -- needs **python 2.7**.) Read `rlexe.xml` to load test metadata and honor `compile-flags`. Run all loaded tests and print summary at the end. Note this is only one step towards current rl test harness. There are lots of test harness features not implemented yet.
8 participants
Use
argparsemodule to handle command line arguments. (Now this alsoworks on Windows -- needs python 2.7.)
Read
rlexe.xmlto load test metadata and honorcompile-flags.Run all loaded tests and print summary at the end.
Note this is only one step towards current rl test harness. There are lots
of test harness features not implemented yet.