[foozzie] Initial correctness fuzzer harness.

[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- Hash source file names in error case
- Bundle script in out directory with executables
- Some suppressions are tied to already fixed bugs. We'll keep it like that for now to test
removing those suppressions in production later.

BUG=chromium:673246
NOTRY=true

Review-Url: https://codereview.chromium.org/2578503003
Cr-Commit-Position: refs/heads/master@{#41789}
Committed: https://chromium.googlesource.com/v8/v8/+/e669816e1fcb4767771d482c58f5e1d51c01e4bc

[Michael Achenbach, 2016-12-16 15:01:57]

Description was changed from

==========
[foozzie] Initial correctness ruzzer harness.

BUG=
==========

to

==========
[foozzie] Initial correctness fuzzer harness.

BUG=chromium:673246
==========

[Michael Achenbach, 2016-12-16 15:01:57]

machenbach@chromium.org changed reviewers:
+ tandrii@chromium.org

[Michael Achenbach, 2016-12-16 15:02:54]

@tandrii: Please take a preliminary look. Mostly for python readability
consistency. Not so much for v8 things.

[tandrii(chromium), 2016-12-16 16:46:26]

two files removed, the rest in a next sitting.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
File tools/foozzie/v8_commands.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
tools/foozzie/v8_commands.py:32: popen_args = [ c for c in args if c != "" ]
nit-nit: space after [ and before ]

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
tools/foozzie/v8_commands.py:44: def kill_process(process, timeout_result):
why args if this func is already inside Execute closure? I'd just declerate
timeout_result above it, OR move this func outside.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
tools/foozzie/v8_commands.py:45: timeout_result[0] = True
this code relies on this line being atomic, as well as reading it on line 61.
It's true in cPython 2.7, but it's still better to pass around threading.Event,
this line would then say event.set(), and line 61 would use event.is_set()

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
File tools/foozzie/v8_foozzie.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:46: FAILURE_HEADER_TEMPLATE = """#
Nit: instead of """# i'd do:
"""
# 
....
""".strip()

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:91: help='optional path to first d8 executable,
default: side-by-side')
what does "side-by-side" mean?

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:108: assert (os.path.exists(options.testcase) and
if x:
  parser.error()
is nicer, but assert is certainly shorter and easier to read. I suppose this
script isn't supposed to be called manually frequently, so I think assert is
fine.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:182: return 2
i'd make 2 a meaningful constant, perhaps 0 too.

[Michael Achenbach, 2016-12-19 08:42:45]

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
File tools/foozzie/v8_commands.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
tools/foozzie/v8_commands.py:32: popen_args = [ c for c in args if c != "" ]
On 2016/12/16 16:46:26, tandrii(chromium) wrote:
> nit-nit: space after [ and before ]

Done.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
tools/foozzie/v8_commands.py:44: def kill_process(process, timeout_result):
On 2016/12/16 16:46:26, tandrii(chromium) wrote:
> why args if this func is already inside Execute closure? I'd just declerate
> timeout_result above it, OR move this func outside.

Done.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_comman...
tools/foozzie/v8_commands.py:45: timeout_result[0] = True
On 2016/12/16 16:46:26, tandrii(chromium) wrote:
> this code relies on this line being atomic, as well as reading it on line 61.
> It's true in cPython 2.7, but it's still better to pass around
threading.Event,
> this line would then say event.set(), and line 61 would use event.is_set()

Done.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
File tools/foozzie/v8_foozzie.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:46: FAILURE_HEADER_TEMPLATE = """#
On 2016/12/16 16:46:26, tandrii(chromium) wrote:
> Nit: instead of """# i'd do:
> """
> # 
> ....
> """.strip()

Done.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:91: help='optional path to first d8 executable,
default: side-by-side')
On 2016/12/16 16:46:26, tandrii(chromium) wrote:
> what does "side-by-side" mean?

Clarified.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:108: assert (os.path.exists(options.testcase) and
On 2016/12/16 16:46:26, tandrii(chromium) wrote:
> if x:
>   parser.error()
> is nicer, but assert is certainly shorter and easier to read. I suppose this
> script isn't supposed to be called manually frequently, so I think assert is
> fine.

The script is designed to be called from automated tooling. parser.error()
actually throws SystemExit, while assert throws assertion error.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:182: return 2
On 2016/12/16 16:46:26, tandrii(chromium) wrote:
> i'd make 2 a meaningful constant, perhaps 0 too.

Done.

[tandrii(chromium), 2016-12-19 08:43:33]

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
File tools/foozzie/v8_foozzie.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:247: 'configs': '',
nitty IMO: % dict(configs='', sources='', suppression='')
it's handy if one's editor checks variables usage and highlights likely
misspelled ones.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
File tools/foozzie/v8_suppressions.js (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.js:5: // This file is loaded before each
correctness test cases and after v8_mock.js.
s/cases/case

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
File tools/foozzie/v8_suppressions.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:75: # Use uncompiled regular expressions.
why? ah, i guessed correctly; maybe:
Use uncompiled regular expressions, they will be compiled later.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:128: for i in range(len(lines) - 1):
xrange

but i'd do this:

for i, l in enumerate(lines):
  yield l, lines[i+1] if i + 1 < len(lines) else None

or less efficient but readable

senti = lines + [None]
for i, l in enumerate(lines):
  yield l, senti[i+1]

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:137: try:
# shorter, and no need to catch KeyboardInterrupt
if lookahead is not None and CARET_RE.match(lookahead):
  continue

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:174: for j, g in enumerate(match1.groups()):
return match1.groups() == match2.groups()  # tuple comparison

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:189: return True
return all(not e.match(line) for e in ignore)

[Michael Achenbach, 2016-12-19 09:42:19]

Done.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
File tools/foozzie/v8_foozzie.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_foozzi...
tools/foozzie/v8_foozzie.py:247: 'configs': '',
On 2016/12/19 08:43:32, tandrii(chromium) wrote:
> nitty IMO: % dict(configs='', sources='', suppression='')
> it's handy if one's editor checks variables usage and highlights likely
> misspelled ones.

Done. Also for the other dicts here.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
File tools/foozzie/v8_suppressions.js (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.js:5: // This file is loaded before each
correctness test cases and after v8_mock.js.
On 2016/12/19 08:43:32, tandrii(chromium) wrote:
> s/cases/case

Done.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
File tools/foozzie/v8_suppressions.py (right):

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:75: # Use uncompiled regular expressions.
On 2016/12/19 08:43:33, tandrii(chromium) wrote:
> why? ah, i guessed correctly; maybe:
> Use uncompiled regular expressions, they will be compiled later.

Done.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:128: for i in range(len(lines) - 1):
On 2016/12/19 08:43:32, tandrii(chromium) wrote:
> xrange
> 
> but i'd do this:
> 
> for i, l in enumerate(lines):
>   yield l, lines[i+1] if i + 1 < len(lines) else None
> 
> or less efficient but readable
> 
> senti = lines + [None]
> for i, l in enumerate(lines):
>   yield l, senti[i+1]

Done, even nicer :)

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:137: try:
On 2016/12/19 08:43:33, tandrii(chromium) wrote:
> # shorter, and no need to catch KeyboardInterrupt
> if lookahead is not None and CARET_RE.match(lookahead):
>   continue

Done. Removed the dead code :)

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:174: for j, g in enumerate(match1.groups()):
On 2016/12/19 08:43:33, tandrii(chromium) wrote:
> return match1.groups() == match2.groups()  # tuple comparison

Done. Except we can't just return - only in True case. In False case continue
trying other regexps, maybe there's a better one.

https://codereview.chromium.org/2578503003/diff/60001/tools/foozzie/v8_suppre...
tools/foozzie/v8_suppressions.py:189: return True
On 2016/12/19 08:43:33, tandrii(chromium) wrote:
> return all(not e.match(line) for e in ignore)

Done.

[tandrii(chromium), 2016-12-19 09:45:31]

lgtm python-wise

[Michael Achenbach, 2016-12-19 09:49:09]

Description was changed from

==========
[foozzie] Initial correctness fuzzer harness.

BUG=chromium:673246
==========

to

==========
[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- hash source file names in error case
- bundle script in out directory with executables

BUG=chromium:673246
==========

[Michael Achenbach, 2016-12-19 09:51:08]

Description was changed from

==========
[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- hash source file names in error case
- bundle script in out directory with executables

BUG=chromium:673246
==========

to

==========
[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- Hash source file names in error case
- Bundle script in out directory with executables
- Some suppressions are tied to already fixed bugs. We'll keep it like that for
now to test
removing those suppressions in production later.

BUG=chromium:673246
==========

[Michael Achenbach, 2016-12-19 09:52:03]

machenbach@chromium.org changed reviewers:
+ bmeurer@chromium.org, jarin@chromium.org

[Michael Achenbach, 2016-12-19 09:52:04]

PTAL bmeurer and/or jarin for V8'ish things and Javascript files.

[Benedikt Meurer, 2016-12-19 09:52:53]

LGTM (rubber-stamped)

[Jarin, 2016-12-19 09:56:40]

lgtm for *.js files.

[Michael Achenbach, 2016-12-19 10:00:33]

Description was changed from

==========
[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- Hash source file names in error case
- Bundle script in out directory with executables
- Some suppressions are tied to already fixed bugs. We'll keep it like that for
now to test
removing those suppressions in production later.

BUG=chromium:673246
==========

to

==========
[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- Hash source file names in error case
- Bundle script in out directory with executables
- Some suppressions are tied to already fixed bugs. We'll keep it like that for
now to test
removing those suppressions in production later.

BUG=chromium:673246
NOTRY=true
==========

[Michael Achenbach, 2016-12-19 10:00:43]

The CQ bit was checked by machenbach@chromium.org

[commit-bot: I haz the power, 2016-12-19 10:00:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 10:03:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-19 10:03:55]

Try jobs failed on following builders:
  v8_presubmit on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/30919)

[Michael Achenbach, 2016-12-19 10:11:45]

The CQ bit was checked by machenbach@chromium.org

[Michael Achenbach, 2016-12-19 10:11:46]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmeurer@chromium.org, jarin@chromium.org, tandrii@chromium.org
Link to the patchset: https://codereview.chromium.org/2578503003/#ps120001
(title: "Presubmit")

[commit-bot: I haz the power, 2016-12-19 10:11:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-19 10:13:49]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1482142305959200,
"parent_rev": "815f91c0ed7bd602246feeb0edcdf01633dbd914", "commit_rev":
"e669816e1fcb4767771d482c58f5e1d51c01e4bc"}

[commit-bot: I haz the power, 2016-12-19 10:13:55]

Description was changed from

==========
[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- Hash source file names in error case
- Bundle script in out directory with executables
- Some suppressions are tied to already fixed bugs. We'll keep it like that for
now to test
removing those suppressions in production later.

BUG=chromium:673246
NOTRY=true
==========

to

==========
[foozzie] Initial correctness fuzzer harness.

Initial version of the correctness fuzzer harness for manual testing
and experiments.

For automated usage, some outstanding TODOs are left in the code. E.g.
- Hash source file names in error case
- Bundle script in out directory with executables
- Some suppressions are tied to already fixed bugs. We'll keep it like that for
now to test
removing those suppressions in production later.

BUG=chromium:673246
NOTRY=true

Review-Url: https://codereview.chromium.org/2578503003
Cr-Commit-Position: refs/heads/master@{#41789}
Committed:
https://chromium.googlesource.com/v8/v8/+/e669816e1fcb4767771d482c58f5e1d51c0...
==========

[commit-bot: I haz the power, 2016-12-19 10:13:56]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/v8/v8/+/e669816e1fcb4767771d482c58f5e1d51c0...