Skip to content
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.

Sign up for GitHub

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

[lit] Fix some issues from --per-test-coverage #65242

Merged
merged 1 commit into from
Sep 14, 2023
Merged

[lit] Fix some issues from --per-test-coverage #65242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 40 additions & 25 deletions llvm/utils/lit/lit/TestRunner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,14 @@ def __init__(self, command, message):
kPdbgRegex = "%dbg\\(([^)'\"]*)\\)(.*)"


def buildPdbgCommand(msg, cmd):
res = f"%dbg({msg}) {cmd}"
assert re.fullmatch(
kPdbgRegex, res
), f"kPdbgRegex expected to match actual %dbg usage: {res}"
return res


class ShellEnvironment(object):

"""Mutable shell environment containing things like CWD and env vars.
Expand Down Expand Up @@ -985,7 +993,7 @@ def _executeShCmd(cmd, shenv, results, timeoutHelper):
def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
cmds = []
for i, ln in enumerate(commands):
match = re.match(kPdbgRegex, ln)
match = re.fullmatch(kPdbgRegex, ln)
if match:
command = match.group(2)
ln = commands[i] = match.expand(": '\\1'; \\2" if command else ": '\\1'")
Expand Down Expand Up @@ -1067,25 +1075,10 @@ def executeScriptInternal(test, litConfig, tmpBase, commands, cwd):
def executeScript(test, litConfig, tmpBase, commands, cwd):
bashPath = litConfig.getBashPath()
isWin32CMDEXE = litConfig.isWindows and not bashPath
coverage_index = 0 # Counter for coverage file index
script = tmpBase + ".script"
if isWin32CMDEXE:
script += ".bat"

# Set unique LLVM_PROFILE_FILE for each run command
for j, ln in enumerate(commands):
match = re.match(kPdbgRegex, ln)
if match:
command = match.group(2)
commands[j] = match.expand(": '\\1'; \\2" if command else ": '\\1'")
if litConfig.per_test_coverage:
# Extract the test case name from the test object
test_case_name = test.path_in_suite[-1]
test_case_name = test_case_name.rsplit(".", 1)[0] # Remove the file extension
llvm_profile_file = f"{test_case_name}{coverage_index}.profraw"
commands[j] = f"export LLVM_PROFILE_FILE={llvm_profile_file} && {commands[j]}"
coverage_index += 1

# Write script file
mode = "w"
open_kwargs = {}
Expand All @@ -1096,7 +1089,7 @@ def executeScript(test, litConfig, tmpBase, commands, cwd):
f = open(script, mode, **open_kwargs)
if isWin32CMDEXE:
for i, ln in enumerate(commands):
match = re.match(kPdbgRegex, ln)
match = re.fullmatch(kPdbgRegex, ln)
if match:
command = match.group(2)
commands[i] = match.expand(
Expand All @@ -1109,7 +1102,7 @@ def executeScript(test, litConfig, tmpBase, commands, cwd):
f.write("\n@if %ERRORLEVEL% NEQ 0 EXIT\n".join(commands))
else:
for i, ln in enumerate(commands):
match = re.match(kPdbgRegex, ln)
match = re.fullmatch(kPdbgRegex, ln)
if match:
command = match.group(2)
commands[i] = match.expand(": '\\1'; \\2" if command else ": '\\1'")
Expand Down Expand Up @@ -1837,13 +1830,7 @@ def _handleCommand(cls, line_number, line, output, keyword):
if not output or not output[-1].add_continuation(line_number, keyword, line):
if output is None:
output = []
pdbg = "%dbg({keyword} at line {line_number})".format(
keyword=keyword, line_number=line_number
)
assert re.match(
kPdbgRegex + "$", pdbg
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the $ dropped in the buildPdbgCommand?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question.

Originally, my goal was for the assert to mimic how kPdbgRegex will actually be used later when expanding %dbg substitutions. That's the usage that the assert is trying to verify will work correctly. Thus, this patch changes the assert in two ways: the searched string contains all of %dbg(...) cmd-line instead of just %dbg(...), and it does not use $.

However, now that you ask, I think the assert should be stricter. For example, D154987 proposes to extend kPdbgRegex to permit newlines, which might appear in lit.run(cmd) in PYTHON directives, as in the example in that review's summary. Using $ in the assert here would have caught that existing deficiency of kPdbgRegex: its .* doesn't match beyond the first newline.

Instead of $, what do you think of using re.fullmatch in the assert? That is, buildPdbgCommand expects kPdbgRegex to match the entire string it's building.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went ahead and applied that change and wrote a commit log to explain it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but does it mean you'll just rebase and push to the repo using pure git? If using the github tool AFAIK you can only merge and squash on the LLVM project so the commit message won't show up unless you modify the PR message. I might be wrong though, I haven't interacted much with github.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least from the web UI you have the chance to edit the final commit message. So you can combine them there if you're ok with it all being one commit.

(otherwise yes manually push some of it)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Yes, I performed most steps from the git command line: squash commits and commit logs together, rebase onto the latest main, and force push to the PR. I then clicked "Squash and merge" and copied and pasted my new commit log there. The commit log it initially offered was the original comment I posted for this PR, but the focus of the PR has shifted since then. It would be nicer if it offered the current commit logs, squashed in the same way a git merge --squash would squash them.

I don't know how to accomplish the rebase onto main via the github web UI... even though it seems like that's almost always going to be required given how frequently new commits show up on main. That means I needed to work from the command line anyway, so it seemed more practical to handle squashing there too.

Please let me know if I've overlooked best practices for using the github web UI.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/about-merge-methods-on-github#squashing-your-merge-commits it seems squash and merge is actually a squash and rebase (it makes it clear it's a fast forward).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The web UI squash-and-merge will automatically rebase to current HEAD.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarifications. For some reason, I thought it previously rejected one when I wasn't caught up to main, but perhaps I had conflicts then. I'll try again next time.

Copy link
Collaborator Author

@jdenny-ornl jdenny-ornl Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, it worked fine in another PR that had no conflicts.

), "kPdbgRegex expected to match actual %dbg usage"
line = "{pdbg} {real_command}".format(pdbg=pdbg, real_command=line)
line = buildPdbgCommand(f"{keyword} at line {line_number}", line)
output.append(CommandDirective(line_number, line_number, keyword, line))
return output

Expand Down Expand Up @@ -2048,6 +2035,27 @@ def parseIntegratedTestScript(test, additional_parsers=[], require_script=True):

def _runShTest(test, litConfig, useExternalSh, script, tmpBase):
def runOnce(execdir):
# Set unique LLVM_PROFILE_FILE for each run command
if litConfig.per_test_coverage:
# Extract the test case name from the test object, and remove the
# file extension.
test_case_name = test.path_in_suite[-1]
test_case_name = test_case_name.rsplit(".", 1)[0]
coverage_index = 0 # Counter for coverage file index
for i, ln in enumerate(script):
match = re.fullmatch(kPdbgRegex, ln)
if match:
dbg = match.group(1)
command = match.group(2)
else:
command = ln
profile = f"{test_case_name}{coverage_index}.profraw"
coverage_index += 1
command = f"export LLVM_PROFILE_FILE={profile}; {command}"
if match:
command = buildPdbgCommand(dbg, command)
hnrklssn marked this conversation as resolved.
Show resolved Hide resolved
script[i] = command

if useExternalSh:
res = executeScript(test, litConfig, tmpBase, script, execdir)
else:
Expand All @@ -2071,7 +2079,14 @@ def runOnce(execdir):
# Re-run failed tests up to test.allowed_retries times.
execdir = os.path.dirname(test.getExecPath())
attempts = test.allowed_retries + 1
scriptInit = script
for i in range(attempts):
# runOnce modifies script, but applying the modifications again to the
# result can corrupt script, so we restore the original upon a retry.
# A cleaner solution would be for runOnce to encapsulate operating on a
# copy of script, but we actually want it to modify the original script
# so we can print the modified version under "Script:" below.
script = scriptInit[:]
res = runOnce(execdir)
if isinstance(res, lit.Test.Result):
return res
Expand Down
5 changes: 4 additions & 1 deletion llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/lit.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ import os

config.name = "per-test-coverage-by-lit-cfg"
config.suffixes = [".py"]
config.test_format = lit.formats.ShTest(execute_external=True)
config.test_format = lit.formats.ShTest(
execute_external=eval(lit_config.params.get("execute_external")),
preamble_commands=["%{python} %s | FileCheck -DINDEX=0 %s"]
)
lit_config.per_test_coverage = True
config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))
5 changes: 3 additions & 2 deletions llvm/utils/lit/tests/Inputs/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Check that the environment variable is set correctly
# RUN: %{python} %s | FileCheck %s
# RUN: %{python} %s | FileCheck -DINDEX=1 %s
# RUN: %{python} %s | FileCheck -DINDEX=2 %s

# Python script to read the environment variable
# and print its value
Expand All @@ -8,4 +9,4 @@
llvm_profile_file = os.environ.get('LLVM_PROFILE_FILE')
print(llvm_profile_file)

# CHECK: per-test-coverage-by-lit-cfg0.profraw
# CHECK: per-test-coverage-by-lit-cfg[[INDEX]].profraw
6 changes: 4 additions & 2 deletions llvm/utils/lit/tests/Inputs/per-test-coverage/lit.cfg
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import os

config.name = "per-test-coverage"
config.suffixes = [".py"]
config.test_format = lit.formats.ShTest(execute_external=True)
config.test_format = lit.formats.ShTest(
execute_external=eval(lit_config.params.get("execute_external")),
preamble_commands=["%{python} %s | FileCheck -DINDEX=0 %s"]
)
config.substitutions.append(("%{python}", '"%s"' % (sys.executable)))

5 changes: 3 additions & 2 deletions llvm/utils/lit/tests/Inputs/per-test-coverage/per-test-coverage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Check that the environment variable is set correctly
# RUN: %{python} %s | FileCheck %s
# RUN: %{python} %s | FileCheck -DINDEX=1 %s
# RUN: %{python} %s | FileCheck -DINDEX=2 %s

# Python script to read the environment variable
# and print its value
Expand All @@ -8,4 +9,4 @@
llvm_profile_file = os.environ.get('LLVM_PROFILE_FILE')
print(llvm_profile_file)

# CHECK: per-test-coverage0.profraw
# CHECK: per-test-coverage[[INDEX]].profraw
12 changes: 12 additions & 0 deletions llvm/utils/lit/tests/allow-retries.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,15 @@
# RUN: rm -f %t.counter
# RUN: %{lit} %{inputs}/test_retry_attempts/test.py -Dcounter=%t.counter -Dpython=%{python} | FileCheck --check-prefix=CHECK-TEST6 %s
# CHECK-TEST6: Passed With Retry: 1

# This test checks that --per-test-coverage doesn't accumulate inserted
# LLVM_PROFILE_FILE= commands across retries.
#
# RUN: rm -f %t.counter
# RUN: %{lit} -a %{inputs}/test_retry_attempts/test.py --per-test-coverage\
# RUN: -Dcounter=%t.counter -Dpython=%{python} | \
# RUN: FileCheck --check-prefix=CHECK-TEST7 %s
# CHECK-TEST7: Command Output (stdout):
# CHECK-TEST7: LLVM_PROFILE_FILE=
# CHECK-TEST7-NOT: LLVM_PROFILE_FILE=
# CHECK-TEST7: Passed With Retry: 1
26 changes: 22 additions & 4 deletions llvm/utils/lit/tests/per-test-coverage-by-lit-cfg.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
# Test if lit_config.per_test_coverage in lit.cfg sets individual test case coverage.

# RUN: %{lit} -a -v %{inputs}/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py \
# RUN: | FileCheck -match-full-lines %s
#
# CHECK: PASS: per-test-coverage-by-lit-cfg :: per-test-coverage-by-lit-cfg.py ({{[^)]*}})
# RUN: %{lit} -a -vv -Dexecute_external=False \
# RUN: %{inputs}/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py | \
# RUN: FileCheck -DOUT=stdout %s

# RUN: %{lit} -a -vv -Dexecute_external=True \
# RUN: %{inputs}/per-test-coverage-by-lit-cfg/per-test-coverage-by-lit-cfg.py | \
# RUN: FileCheck -DOUT=stderr %s

# CHECK: {{^}}PASS: per-test-coverage-by-lit-cfg :: per-test-coverage-by-lit-cfg.py ({{[^)]*}})
# CHECK: Command Output ([[OUT]]):
# CHECK-NEXT: --
# CHECK: export
# CHECK: LLVM_PROFILE_FILE=per-test-coverage-by-lit-cfg0.profraw
# CHECK: per-test-coverage-by-lit-cfg.py
# CHECK: {{RUN}}: at line 2
# CHECK: export
# CHECK: LLVM_PROFILE_FILE=per-test-coverage-by-lit-cfg1.profraw
# CHECK: per-test-coverage-by-lit-cfg.py
# CHECK: {{RUN}}: at line 3
# CHECK: export
# CHECK: LLVM_PROFILE_FILE=per-test-coverage-by-lit-cfg2.profraw
# CHECK: per-test-coverage-by-lit-cfg.py
26 changes: 22 additions & 4 deletions llvm/utils/lit/tests/per-test-coverage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,24 @@
# Test LLVM_PROFILE_FILE is set when --per-test-coverage is passed to command line.

# RUN: %{lit} -a -v --per-test-coverage %{inputs}/per-test-coverage/per-test-coverage.py \
# RUN: | FileCheck -match-full-lines %s
#
# CHECK: PASS: per-test-coverage :: per-test-coverage.py ({{[^)]*}})
# RUN: %{lit} -a -vv --per-test-coverage -Dexecute_external=False \
# RUN: %{inputs}/per-test-coverage/per-test-coverage.py | \
# RUN: FileCheck -DOUT=stdout %s

# RUN: %{lit} -a -vv --per-test-coverage -Dexecute_external=True \
# RUN: %{inputs}/per-test-coverage/per-test-coverage.py | \
# RUN: FileCheck -DOUT=stderr %s

# CHECK: {{^}}PASS: per-test-coverage :: per-test-coverage.py ({{[^)]*}})
# CHECK: Command Output ([[OUT]]):
# CHECK-NEXT: --
# CHECK: export
# CHECK: LLVM_PROFILE_FILE=per-test-coverage0.profraw
# CHECK: per-test-coverage.py
# CHECK: {{RUN}}: at line 2
# CHECK: export
# CHECK: LLVM_PROFILE_FILE=per-test-coverage1.profraw
# CHECK: per-test-coverage.py
# CHECK: {{RUN}}: at line 3
# CHECK: export
# CHECK: LLVM_PROFILE_FILE=per-test-coverage2.profraw
# CHECK: per-test-coverage.py