I think he means that it’s likely diff is already being used as part of a pipeline (which would be broken by this).  Did you confirm that it’s not?<br><br>Also, did you test this with both Python 2 and Python 3?<br><div class="gmail_quote"><div dir="ltr">On Tue, Nov 7, 2017 at 6:30 AM Ying Yi via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">MaggieYi added a comment.<br>
<br>
<br>
<br>
> 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?<br>
<br>
I think it tests the lit builtin command for the following reasons:<br>
<br>
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.<br>
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.<br>
3. Since all the failed tests print the builtins commands error information, the successful tests should be run using lit builtins shell.<br>
<br>
What do you think?<br>
<br>
> 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).<br>
<br>
Thanks, I have updated the patch to solve this issue.<br>
<br>
<br>
<br>
================<br>
Comment at: lit/lit/TestRunner.py:566-569<br>
+    if cmd.commands[0].args[0] == 'mkdir':<br>
+        if len(cmd.commands) != 1:<br>
+            raise InternalShellError(cmd.commands[0], "Unsupported: 'mkdir' "<br>
+                                     "cannot be part of a pipeline")<br>
----------------<br>
rnk wrote:<br>
> 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.<br>
It is a good idea.<br>
<br>
<br>
================<br>
Comment at: lit/lit/TestRunner.py:573-576<br>
+    if cmd.commands[0].args[0] == 'diff':<br>
+        if len(cmd.commands) != 1:<br>
+            raise InternalShellError(cmd.commands[0], "Unsupported: 'diff' "<br>
+                                     "cannot be part of a pipeline")<br>
----------------<br>
rnk wrote:<br>
> I'd be surprised if this isn't used somewhere.<br>
I'm sorry, I'm not sure about this. Could you possibly give me an explanation? Thanks.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D39567" rel="noreferrer" target="_blank">https://reviews.llvm.org/D39567</a><br>
<br>
<br>
<br>
</blockquote></div>