Closed Bug 1459880 Opened Last year Closed Last year

Various refactoring in the jsshell test harness

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: Ms2ger, Assigned: Ms2ger)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 1 obsolete file)

No description provided.
(This change did not end up being as useful for my future work as I thought, so I don't feel strongly about whether it should land or not.)
Attachment #8973982 - Flags: review?(jdemooij)
Comment on attachment 8973976 [details] [diff] [review]
part a: Use a |with| statement to open exclusion files

Review of attachment 8973976 [details] [diff] [review]:
-----------------------------------------------------------------

I may forward reviews here to someone more familiar with the test harness, but this one at least is simple enough.
Attachment #8973976 - Flags: review?(jdemooij) → review+
Attachment #8973977 - Flags: review?(jdemooij) → review+
Attachment #8973978 - Flags: review?(jdemooij) → review+
Attachment #8973979 - Flags: review?(jdemooij) → review+
Attachment #8973980 - Flags: review?(jdemooij) → review+
Comment on attachment 8973981 [details] [diff] [review]
part f: Use an iterative algorithm for |RefTestCase.prefix_command|

Review of attachment 8973981 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling review per:

<jandem> Ms2ger: why do we need the os.path.exists check now in prefix_command?
<Ms2ger> jandem, oh, good point, I didn't mean to put that in that patch
Attachment #8973981 - Flags: review?(jdemooij)
Comment on attachment 8973981 [details] [diff] [review]
part f: Use an iterative algorithm for |RefTestCase.prefix_command|

Review of attachment 8973981 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/tests/lib/tests.py
@@ +177,5 @@
> +        while path != '':
> +            assert path != '/'
> +            path = os.path.dirname(path)
> +            shell_path = os.path.join(path, 'shell.js')
> +            if os.path.exists(shell_path):

Remove this check for now, doesn't belong in this patch.
Comment on attachment 8973981 [details] [diff] [review]
part f: Use an iterative algorithm for |RefTestCase.prefix_command|

Review of attachment 8973981 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the if-statement removed.
Attachment #8973981 - Flags: review+
Comment on attachment 8973982 [details] [diff] [review]
part g: Introduce a PathOptions class for included and excluded paths

Review of attachment 8973982 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah this is definitely more readable.
Attachment #8973982 - Flags: review?(jdemooij) → review+
Comment on attachment 8974020 [details] [diff] [review]
part f: Use an iterative algorithm for |RefTestCase.prefix_command| (v2)

r=jandem in comment 11
Attachment #8974020 - Flags: review?(jdemooij) → review+
Pushed by Ms2ger@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1a42cd32776
part a: Use a |with| statement to open exclusion files; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c388fb0d97f4
part b: Remove unused |reldir| argument from |load_reftests|; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/f70e5f5fcb0d
part c: Consolidate the arguments to |_find_all_js_files|; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/532eadb71ad0
part d: Correct the documentation for |load_tests|; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c4c664ec99f
part e: Merge the RefTest and RefTestCase classes; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/aabb6982d6c9
part f: Use an iterative algorithm for |RefTestCase.prefix_command|; r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6dea32cd53b
part g: Introduce a PathOptions class for included and excluded paths; r=jandem
Blocks: wasm-wpt
You need to log in before you can comment on or make changes to this bug.