[llvm-lit] Add precommit test to verify current behavior of glob expansion in lit's internal shell #106325
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit introduces a new test file, shtest-glob.py, to the lit testing framework. The test is designed to verify the handling of glob patterns within the internal shell, specifically checking the behavior of commands like echo when glob patterns are present. The test includes example files, example_file1.txt and example_file2.txt, which are used to validate whether the glob expansion is correctly performed or remains unexpanded as expected in certain cases. This addition helps ensure that glob pattern processing behaves consistently across different shell commands in lit.
|
@llvm/pr-subscribers-testing-tools Author: None (Harini0924) ChangesThis patch introduces a precommit test to verify the current behavior of glob expansion in lit's internal shell. The motivation for this test stems from an issue encountered during the BOLT test suite when running with the lit internal shell using the command:
During execution, the following error was observed: The While this patch doesn't fix the issue, it helps in understanding and verifying the current behavior. The feedback I received from this PR suggests using Request for Feedback: If such tests are recommended, I would also appreciate advice on the best approach to implement them, considering the existing framework and the way glob expansion is expected to function in the internal shell. Should these tests confirm that the current implementation passes, or are there specific edge cases I should be aware of? Next Steps: In my follow-up PR, I plan to address the UNRESOLVED error by expanding the entire command, ensuring correct and consistent behavior across all commands. The current test checks for an unresolved issue with the glob expansion, specifically looking for a This change is relevant for [RFC] Enabling the Lit Internal Shell by Default Full diff: https://github.com/llvm/llvm-project/pull/106325.diff 4 Files Affected:
diff --git a/llvm/utils/lit/tests/Inputs/shtest-glob/example_file1.txt b/llvm/utils/lit/tests/Inputs/shtest-glob/example_file1.txt
new file mode 100644
index 00000000000000..1bf50291a824a6
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-glob/example_file1.txt
@@ -0,0 +1,2 @@
+## This is the first example file used for testing glob pattern matching.
+RUN: This is the first example file.
diff --git a/llvm/utils/lit/tests/Inputs/shtest-glob/example_file2.txt b/llvm/utils/lit/tests/Inputs/shtest-glob/example_file2.txt
new file mode 100644
index 00000000000000..7263ea1dccf4a8
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-glob/example_file2.txt
@@ -0,0 +1,2 @@
+## This is the second example file used for testing glob pattern matching.
+RUN: This is the second example file.
diff --git a/llvm/utils/lit/tests/Inputs/shtest-glob/glob-echo.txt b/llvm/utils/lit/tests/Inputs/shtest-glob/glob-echo.txt
new file mode 100644
index 00000000000000..31fc03f22bd5f9
--- /dev/null
+++ b/llvm/utils/lit/tests/Inputs/shtest-glob/glob-echo.txt
@@ -0,0 +1,2 @@
+## Tests glob pattern expansion by listing matching files.
+# RUN: echo %{inputs}/shtest-glob/example_file*.txt
diff --git a/llvm/utils/lit/tests/shtest-glob.py b/llvm/utils/lit/tests/shtest-glob.py
new file mode 100644
index 00000000000000..3d59e681cca4f9
--- /dev/null
+++ b/llvm/utils/lit/tests/shtest-glob.py
@@ -0,0 +1,9 @@
+## Tests glob pattern handling in echo command.
+
+# RUN: not %{lit} -a -v %{inputs}/shtest-glob \
+# RUN: | FileCheck -dump-input=fail -match-full-lines %s
+#
+# END.
+
+# CHECK: UNRESOLVED: shtest-glob :: glob-echo.txt ({{[^)]*}})
+# CHECK: TypeError: string argument expected, got 'GlobItem'
|
I forgot to add the lit.cfg file in my previous commit.
|
✅ With the latest revision this PR passed the Python code formatter. |
arichardson
reviewed
Aug 28, 2024
arichardson
reviewed
Aug 28, 2024
arichardson
approved these changes
Aug 28, 2024
Renamed `example_file1.txt` and `example_file2.txt` to `.input` files to avoid `RUN` lines in test files and ensure they are not executed as test scripts. Added `glob-mkdir.txt` to test `mkdir` with glob patterns, ensuring failure when directories exist. Modified `shtest-glob.py` to include checks for both `echo` and `mkdir` command tests. Ensured that the `mkdir` command test is expected to fail, confirming proper error handling for existing paths.
|
I will address the code formatter issues later. |
|
@arichardson I followed your suggestion about keeping the tests in separate files to align with the current structure of the lit internal test suite. I created a file |
ilovepi
reviewed
Aug 29, 2024
arichardson
approved these changes
Aug 29, 2024
| # CHECK: TypeError: string argument expected, got 'GlobItem' | ||
|
|
||
| # CHECK: FAIL: shtest-glob :: glob-mkdir.txt ({{[^)]*}}) | ||
| # CHECK: # error: command failed with exit status: 1 |
There was a problem hiding this comment.
I was surprised there is no more specific error here but it looks like the builtin mkdir does not print any output right now. Turns out it doesn't handle EEXISTS the same way as the real mkdir command. I can submit a patch for this once this commit lands.
There was a problem hiding this comment.
A patch to address that would be great, and I’m looking forward to seeing it once this commit lands.
But please do fix the trailing whitespace before committing. |
arichardson
approved these changes
Aug 30, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/5871 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/4327 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/4372 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/2142 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/4460 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/6159 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/7491 Here is the relevant piece of the build log for the reference |
schwitanski
pushed a commit
to RWTH-HPC/llvm-lit-mirror
that referenced
this pull request
Feb 27, 2025
…lob expansion in lit's internal shell" (#106763) Reverts llvm/llvm-project#106325 Broke some Buildbots.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch introduces a precommit test to verify the current behavior of glob expansion in lit's internal shell. The motivation for this test stems from an issue encountered during the BOLT test suite when running with the lit internal shell using the command:
LIT_USE_INTERNAL_SHELL=1 ninja check-boltDuring execution, the following error was observed:
The
executeBuiltinEchofunction in the lit testing framework expects a string to be passed tostdout.write, but it received aGlobItemobject instead. This precommit test is designed to check the current behavior where the glob pattern isn't correctly expanded, leading to thisTypeError.While this patch doesn't fix the issue, it helps in understanding and verifying the current behavior. The feedback I received from this PR suggests using
cmd.args = expand_glob_expressions(cmd.args, shenv.cwd)to match the behavior ofexecuteBuiltinMkdirandexecuteBuiltinRm, but it is recognized that the internal shell should ideally expand globs before calling any built-in command.Request for Feedback:
I'm looking for feedback on how to improve this precommit test, specifically regarding the handling and expansion of glob patterns for commands like mkdir and rm within the internal shell. Currently, the args are expanded at the beginning of these functions, which should ensure proper glob expansion. However, I'd appreciate guidance on whether I should write additional tests to verify that mkdir and rm are handling glob expansions correctly.
If such tests are recommended, I would also appreciate advice on the best approach to implement them, considering the existing framework and the way glob expansion is expected to function in the internal shell. Should these tests confirm that the current implementation passes, or are there specific edge cases I should be aware of?
Next Steps:
In my follow-up PR, I plan to address the UNRESOLVED error by expanding the entire command, ensuring correct and consistent behavior across all commands. The current test checks for an unresolved issue with the glob expansion, specifically looking for a
TypeErrordue to an unexpandedGlobItem. This will be updated to reflect the correct behavior once the issue is resolved.This change is relevant for [RFC] Enabling the Lit Internal Shell by Default