1459880 - Various refactoring in the jsshell test harness

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part a: Use a |with| statement to open exclusion files

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part b: Remove unused |reldir| argument from |load_reftests|

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part c: Consolidate the arguments to |_find_all_js_files|

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part d: Correct the documentation for |load_tests|

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part e: Merge the RefTest and RefTestCase classes

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part f: Use an iterative algorithm for |RefTestCase.prefix_command|

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part g: Introduce a PathOptions class for included and excluded paths

(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.)

Comment 8

[Jan de Mooij [:jandem]]

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.

Comment 9

[Jan de Mooij [:jandem]]

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

Comment 10

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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 11

[Jan de Mooij [:jandem]]

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.

Comment 12

[Jan de Mooij [:jandem]]

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.

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: part f: Use an iterative algorithm for |RefTestCase.prefix_command| (v2)

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8974020 [details] [diff] [review]
part f: Use an iterative algorithm for |RefTestCase.prefix_command| (v2)

r=jandem in comment 11

Comment 15

[Pulsebot]

Pushed by [e-mail]:
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

Comment 16

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c1a42cd32776
https://hg.mozilla.org/mozilla-central/rev/c388fb0d97f4
https://hg.mozilla.org/mozilla-central/rev/f70e5f5fcb0d
https://hg.mozilla.org/mozilla-central/rev/532eadb71ad0
https://hg.mozilla.org/mozilla-central/rev/8c4c664ec99f
https://hg.mozilla.org/mozilla-central/rev/aabb6982d6c9
https://hg.mozilla.org/mozilla-central/rev/e6dea32cd53b