Test harness support for reflect-stringify

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jorendorff, Assigned: jorendorff)

Tracking

Trunk
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
Jesse was interested in using <https://github.com/jorendorff/reflect-stringify> in a fuzz-test reducer.

I hacked up a way to use our test harness to check that the library works on all syntax that we use in our test suite (not counting Function/eval code). I'd like to land this testing support. In the short term, it'll be helpful for me in keeping reflect-stringify working.

Long-term, if Jesse finds the library valuable, we might have to bring reflect-stringify into the tree and support it, but let's worry about that when it happens.
(Assignee)

Comment 1

3 years ago
Created attachment 8682726 [details] [diff] [review]
Part 1: Add "-S" option to disassemble() to omit source notes from the output. Not strictly necessary, but convenient
Attachment #8682726 - Flags: review?(efaustbmo)
(Assignee)

Updated

3 years ago
Assignee: nobody → jorendorff
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8682728 [details] [diff] [review]
Part 2: Test harness support for --test-reflect-stringify
Attachment #8682728 - Flags: review?(efaustbmo)

Comment 3

3 years ago
Comment on attachment 8682726 [details] [diff] [review]
Part 1: Add "-S" option to disassemble() to omit source notes from the output. Not strictly necessary, but convenient

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

::: js/src/shell/js.cpp
@@ +1729,5 @@
> +        AutoStableStringChars linearChars(cx);
> +        if (!linearChars.initTwoByte(cx, linearStr))
> +            return nullptr;
> +        const char16_t* chars = linearChars.twoByteRange().start().get();
> +        

nit: whitespace on blank line

@@ +2113,5 @@
> +    Value* argv;
> +    bool lines;
> +    bool recursive;
> +    bool sourceNotes;
> +    bool parseScript;

As near as I can tell, this is never set, other than by the constructor, and never consulted, except in the loop where it must be false. Can we remove it?
Attachment #8682726 - Flags: review?(efaustbmo) → review+

Comment 4

3 years ago
Comment on attachment 8682728 [details] [diff] [review]
Part 2: Test harness support for --test-reflect-stringify

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

LGTM

::: js/src/tests/jstests.py
@@ +261,5 @@
> +                # expect reflect-stringify to be able to handle it.
> +                test.expect = True
> +                test.random = False
> +                test.slow = False
> +                yield test

Is the reason we don't need similar logic above that we never parse these options for jit_tests unless we run them?
Attachment #8682728 - Flags: review?(efaustbmo) → review+
(Assignee)

Comment 5

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b459ff7821b1b30a3cee11747b4bb3cf6fb0886c
Bug 1221285 - Part 1: Add "-S" option to disassemble() to omit source notes from the output. Not strictly necessary, but convenient. r=efaust.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8969b317c0462dddeb5b9557107c4c278bf0187d
Bug 1221285 - Part 2: Test harness support for --test-reflect-stringify. r=efaust.

Comment 6

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b459ff7821b1
https://hg.mozilla.org/mozilla-central/rev/8969b317c046
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.