[PATCH] D39567: [lit] Implement non-pipelined ‘mkdir’, ‘diff’ and ‘rm’ commands internally

[Ying Yi via Phabricator via llvm-commits]

MaggieYi added a comment.



> 1. I don't see any tests for the case where the commands *succeed*.  Only for the case where they fail.  Shouldn't we have some tests that verify the commands actually work?

I think it tests the lit builtin command for the following reasons:

1. Lit tests enables the use of LIT’s internal shell in ShTests (\llvm\utils\lit\tests\lit.cfg) Line 14: config.test_format = lit.formats.ShTest(execute_external=False) Lit tests use internal shell instead of using external shell.
2. The function _executeShCmd  returns once the shell builtins commands finish. (line 571, 578 and 585 in TestRunner.py) They will not execute the OS command.
3. Since all the failed tests print the builtins commands error information, the successful tests should be run using lit builtins shell.

What do you think?

> 2. Now that we have upwards of 6 builtin commands, I'm starting to think they should be separated into their own file.  For example, `lit/test/ShellBuiltins.py`  We could test them via unittests (e.g. see how `BooleanExpression.py` implements its unit tests).

Thanks, I have updated the patch to solve this issue.



================
Comment at: lit/lit/TestRunner.py:566-569
+    if cmd.commands[0].args[0] == 'mkdir':
+        if len(cmd.commands) != 1:
+            raise InternalShellError(cmd.commands[0], "Unsupported: 'mkdir' "
+                                     "cannot be part of a pipeline")
----------------
rnk wrote:
> Can you refactor the handling of non-pipelined commands, either in this change, or split before or after? I'm imagining a dict of commands and methods that implement builtin shell commands, which would then be ripe for refactoring into a separate shell module or something.
It is a good idea.


================
Comment at: lit/lit/TestRunner.py:573-576
+    if cmd.commands[0].args[0] == 'diff':
+        if len(cmd.commands) != 1:
+            raise InternalShellError(cmd.commands[0], "Unsupported: 'diff' "
+                                     "cannot be part of a pipeline")
----------------
rnk wrote:
> I'd be surprised if this isn't used somewhere.
I'm sorry, I'm not sure about this. Could you possibly give me an explanation? Thanks.


https://reviews.llvm.org/D39567