-
Notifications
You must be signed in to change notification settings - Fork 859
update_lit_tests.py: Do not leak results between modules #8791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
8cbca31
813b20d
bda5f4a
a2bf26b
b345d97
da9ef30
e8b9bf2
c391e1b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,10 @@ | |
| import sys | ||
| import tempfile | ||
|
|
||
| from test import support | ||
|
|
||
| INTERNAL_SEPARATOR = '[update-lit-checks-separator]' | ||
|
|
||
| script_dir = os.path.dirname(__file__) | ||
| script_name = os.path.basename(__file__) | ||
|
|
||
|
|
@@ -108,8 +112,26 @@ def run_command(args, test, tmp, command): | |
| command = command.replace('%s', test) | ||
| command = command.replace('%S', os.path.dirname(test)) | ||
| command = command.replace('%t', tmp) | ||
| command = command.replace('foreach', os.path.join(script_dir, 'foreach.py')) | ||
| return subprocess.check_output(command, shell=True, env=env).decode('utf-8') | ||
|
|
||
| match = re.match(r'^foreach\s+(\S+)\s+(\S+)\s+(.*)$', command) | ||
| if match: | ||
| infile = match.group(1) | ||
| tempfile = match.group(2) | ||
| cmd_rest = match.group(3) | ||
|
|
||
| outputs = [] | ||
| for i, (module, _asserts) in enumerate(support.split_wast(infile)): | ||
| tempname = tempfile + '.' + str(i) | ||
| with open(tempname, 'w') as temp: | ||
| print(module, file=temp) | ||
| new_command = cmd_rest + ' ' + tempname | ||
| out = subprocess.check_output(new_command, shell=True, env=env).decode('utf-8') | ||
| outputs.append(out) | ||
|
|
||
| return f"\n{INTERNAL_SEPARATOR}\n".join(outputs) | ||
| else: | ||
| command = command.replace('foreach', os.path.join(script_dir, 'foreach.py')) | ||
| return subprocess.check_output(command, shell=True, env=env).decode('utf-8') | ||
|
|
||
|
|
||
| def find_end(module, start): | ||
|
|
@@ -205,10 +227,32 @@ def parse_output_fuzz_exec(text, first_named_item): | |
| return [items] | ||
|
|
||
|
|
||
| def get_command_output(args, kind, test, lines, tmp, named_items): | ||
| def split_outputs(output): | ||
| if INTERNAL_SEPARATOR in output: | ||
| return re.split(r'\n?' + re.escape(INTERNAL_SEPARATOR) + r'\n?', output) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this not just return
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh good point, I guess python splitting does work that way. Done. |
||
| else: | ||
| return [output] | ||
|
|
||
|
|
||
| def get_modules_named_items(lines): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment about the structure of the data this is returning.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| modules_text = split_modules('\n'.join(lines)) | ||
| modules_named_items = [] | ||
| for module_text in modules_text: | ||
| named_items = [] | ||
| for line in module_text.split('\n'): | ||
| match = ITEM_RE.match(line) | ||
| if match: | ||
| _, kind, name = indentKindName(match) | ||
| named_items.append((kind, name)) | ||
| modules_named_items.append(named_items) | ||
| return modules_named_items | ||
|
|
||
|
|
||
| def get_command_output(args, kind, test, lines, tmp): | ||
| # Return list of maps from prefixes to lists of module items of the form | ||
| # ((kind, name), [line]). The outer list has an entry for each module. | ||
| command_output = [] | ||
| modules_named_items = get_modules_named_items(lines) | ||
| for line in find_run_lines(test, lines): | ||
| commands = [cmd.strip() for cmd in line.rsplit('|', 1)] | ||
| if (len(commands) > 2 or | ||
|
|
@@ -229,12 +273,31 @@ def get_command_output(args, kind, test, lines, tmp, named_items): | |
|
|
||
| output = run_command(args, test, tmp, commands[0]) | ||
| if prefix: | ||
| if kind == 'wat': | ||
| module_outputs = parse_output_modules(output) | ||
| elif kind == 'fuzz-exec': | ||
| module_outputs = parse_output_fuzz_exec(output, named_items[0]) | ||
| outputs = split_outputs(output) | ||
| module_outputs = [] | ||
| if len(outputs) == 1 and len(modules_named_items) > 1 and kind == 'wat': | ||
| module_outputs = parse_output_modules(outputs[0]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's special about the case of having one output module, more than one named item, and 'wat' output? Can this not be handled by the more general logic below?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, nothing I guess, good point. This is the easiest case to reason about but it does work in the general path too. Done. |
||
| else: | ||
| assert False, "unknown output kind" | ||
| if len(outputs) != len(modules_named_items): | ||
| warn(f'Mismatch between output parts ({len(outputs)}) and ' | ||
| f'input modules ({len(modules_named_items)}).') | ||
| for i, out in enumerate(outputs): | ||
| if i >= len(modules_named_items): | ||
| break | ||
| mod_named_items = modules_named_items[i] | ||
| first_named_item = mod_named_items[0] if mod_named_items else None | ||
| if kind == 'wat': | ||
| mod_out = parse_output_modules(out) | ||
| if mod_out: | ||
| module_outputs.append(mod_out[0]) | ||
| else: | ||
| module_outputs.append([]) | ||
| elif kind == 'fuzz-exec': | ||
| mod_out = parse_output_fuzz_exec(out, first_named_item) | ||
| if mod_out: | ||
| module_outputs.append(mod_out[0]) | ||
| else: | ||
| module_outputs.append([]) | ||
| for i in range(len(module_outputs)): | ||
| if len(command_output) == i: | ||
| command_output.append({}) | ||
|
|
@@ -265,7 +328,7 @@ def update_test(args, test, lines, tmp): | |
| _, kind, name = indentKindName(match) | ||
| named_items.append((kind, name)) | ||
|
|
||
| command_output = get_command_output(args, output_kind, test, lines, tmp, named_items) | ||
| command_output = get_command_output(args, output_kind, test, lines, tmp) | ||
|
|
||
| prefixes = {prefix for module_output in command_output for prefix in module_output.keys()} | ||
| check_line_re = re.compile(r'^\s*;;\s*(' + '|'.join(prefixes) + | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we expect this to ever be used? Wouldn't all uses of
foreachmatch the regex above?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some cases not handled, e.g.
from
gsi-debug.wast. Those are rare so I left them unhandled. Do you think it is worth fully parsing this?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh interesting. Maybe it's not too complicated to handle that by capturing an additional group for everything before the
foreachand prepending it to each of the individual commands? If that doesn't work, adding a TODO to look into this would be enough.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, added that group. I'm not super happy about
\s*foreach(which could in theory match foreach as the suffix of something), but that seems unlikely... and I don't see a nicer way.