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

[MaggieYi]

Summary

The internal shell already supports 'cd', ‘export’ and ‘echo’ commands. This patch adds implementation of non-pipelined ‘mkdir’, ‘diff’ and ‘rm’ commands as the internal shell builtins.

[zturner]

First off, let me say this is very cool. I look forward to the day when all commands are builtin, and we can switch every client to the builtin lit shell.

I have two primary thoughts upon reading the patch:

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?

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).

What do you think?

[zturner]

Update: I actually do see where you're testing that they work, but I'm not convinced that the tests are sufficient. For several reasons.

1. it doesn't test that the builtin command is actually run versus the OS command. It only tests that whatever command is run doesn't fail.

2. It doesn't test that the result of the command is as expected, it only tests that it doesn't return an error. For a somewhat unrealistic illustration of this, imagine that all the commands just returned success without actually doing anything. The test as it is written now would pass.

[MaggieYi]

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).

What do you think?

Hi Zachary,

Many thanks for your help on reviewing the patch and sorry for my late reply.

Yes, you are right. It could be done as a follow-up patch.

Thanks,

[MaggieYi]

Update: I actually do see where you're testing that they work, but I'm not convinced that the tests are sufficient. For several reasons.

1. it doesn't test that the builtin command is actually run versus the OS command. It only tests that whatever command is run doesn't fail.

2. It doesn't test that the result of the command is as expected, it only tests that it doesn't return an error. For a somewhat unrealistic illustration of this, imagine that all the commands just returned success without actually doing anything. The test as it is written now would pass.

Good points. I am working on this issue now.

[rnk]

lit/lit/TestRunner.py

566–569 ↗

(On Diff #121366)

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.

573–576 ↗

(On Diff #121366)

I'd be surprised if this isn't used somewhere.

[MaggieYi]

The patch updated the test (valid-shell.txt) where lit internal shell commands succeeded.

[MaggieYi]

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.

lit/lit/TestRunner.py

566–569 ↗

(On Diff #121366)

It is a good idea.

573–576 ↗

(On Diff #121366)

I'm sorry, I'm not sure about this. Could you possibly give me an explanation? Thanks.

[MaggieYi]

Updated the builtin diff shell to support ‘-u’, ‘-b’ and ‘-w’ options. Now, all llvm, and clang tests passed with both Python 2 and Python 3.

[MaggieYi]

Optimised the patch by using python ‘getopt’ module.

[zturner]

looks good to me, rnk@ are you ok?

[rnk]

lgtm

[MaggieYi]

The test of MC/AsmParser/preserve-comments-crlf.s failed since it uses the unsupported diff option --strip-trailing-cr. I updated the internal diff shell command to support this option.

[MaggieYi]

Reverted rL318911 since it broke the sanitizer-windows.

[MaggieYi]

The previous patch doesn’t support the sequential commands, for example “mkdir existing_folder || true”. This caused sanitizer-windows test failed. The patch has been updated to support the sequential commands.

[rnk]

lgtm

I think the windows asan test cases should probably use mkdir -p instead but I like the changes you made to make the internal shell commands simply print to stderr and return a non-zero exit code. I think that'll be simpler and more readable when writing new test cases with the internal shell. The test will get a FAIL status instead of an UNRESOLVED status.

llvm/trunk/utils/lit/lit/TestRunner.py

 from __future__ import absolute_import
+import difflib
+import functools
+import itertools
+import getopt
 import os, signal, subprocess, sys
 import re
+import stat
 import platform
+import shutil
 import tempfile
 import threading
 
 try:
     from StringIO import StringIO
 except ImportError:
     from io import StringIO
 
def executeBuiltinEcho(cmd, shenv):
 
     for (name, mode, f, path) in opened_files:
         f.close()
 
     if not is_redirected:
         return stdout.getvalue()
     return ""
 
+def executeBuiltinMkdir(cmd, cmd_shenv):
+    """executeBuiltinMkdir - Create new directories."""
+    args = expand_glob_expressions(cmd.args, cmd_shenv.cwd)[1:]
+    try:
+        opts, args = getopt.gnu_getopt(args, 'p')
+    except getopt.GetoptError as err:
+        raise InternalShellError(cmd, "Unsupported: 'mkdir':  %s" % str(err))
+
+    parent = False
+    for o, a in opts:
+        if o == "-p":
+            parent = True
+        else:
+            assert False, "unhandled option"
+
+    if len(args) == 0:
+        raise InternalShellError(cmd, "Error: 'mkdir' is missing an operand")
+
+    for dir in args:
+        if not os.path.isabs(dir):
+            dir = os.path.realpath(os.path.join(cmd_shenv.cwd, dir))
+        if parent:
+            lit.util.mkdir_p(dir)
+        else:
+            try:
+                os.mkdir(dir)
+            except OSError as e:
+                raise InternalShellError(cmd, "Error: 'mkdir' command failed")
+
+def executeBuiltinDiff(cmd, cmd_shenv):
+    """executeBuiltinDiff - Compare files line by line."""
+    args = expand_glob_expressions(cmd.args, cmd_shenv.cwd)[1:]
+    try:
+        opts, args = getopt.gnu_getopt(args, "wbu", ["strip-trailing-cr"])
+    except getopt.GetoptError as err:
+        raise InternalShellError(cmd, "Unsupported: 'diff':  %s" % str(err))
+
+    filelines, filepaths = ([] for i in range(2))
+    ignore_all_space = False
+    ignore_space_change = False
+    unified_diff = False
+    strip_trailing_cr = False
+    for o, a in opts:
+        if o == "-w":
+            ignore_all_space = True
+        elif o == "-b":
+            ignore_space_change = True
+        elif o == "-u":
+            unified_diff = True
+        elif o == "--strip-trailing-cr":
+            strip_trailing_cr = True
+        else:
+            assert False, "unhandled option"
+
+    if len(args) != 2:
+        raise InternalShellError(cmd, "Error:  missing or extra operand")
+
+    different = False
+    try:
+        for file in args:
+            if not os.path.isabs(file):
+                file = os.path.realpath(os.path.join(cmd_shenv.cwd, file))
+            filepaths.append(file)
+            with open(file, 'r') as f:
+                filelines.append(f.readlines())
+
+        def compose2(f, g):
+            return lambda x: f(g(x))
+
+        f = lambda x: x
+        if strip_trailing_cr:
+            f = compose2(lambda line: line.rstrip('\r'), f)
+        if ignore_all_space or ignore_space_change:
+            ignoreSpace = lambda line, separator: separator.join(line.split())
+            ignoreAllSpaceOrSpaceChange = functools.partial(ignoreSpace, separator='' if ignore_all_space else ' ')
+            f = compose2(ignoreAllSpaceOrSpaceChange, f)
+
+        for idx, lines in enumerate(filelines):
+            filelines[idx]= [f(line) for line in lines]
+
+        func = difflib.unified_diff if unified_diff else difflib.context_diff
+        for diff in func(filelines[0], filelines[1], filepaths[0], filepaths[1]):
+            sys.stdout.write(diff)
+            different = True
+    except IOError as e:
+        raise InternalShellError(cmd, "Error: 'diff' command failed")
+
+    if different:
+        raise InternalShellError(cmd, "Error: The contents "
+                                      "of fromfile and tofile are different")
+
+def executeBuiltinRm(cmd, cmd_shenv):
+    """executeBuiltinRm - Removes (deletes) files or directories."""
+    args = expand_glob_expressions(cmd.args, cmd_shenv.cwd)[1:]
+    try:
+        opts, args = getopt.gnu_getopt(args, "frR", ["--recursive"])
+    except getopt.GetoptError as err:
+        raise InternalShellError(cmd, "Unsupported: 'rm':  %s" % str(err))
+
+    force = False
+    recursive = False
+    for o, a in opts:
+        if o == "-f":
+            force = True
+        elif o in ("-r", "-R", "--recursive"):
+            recursive = True
+        else:
+            assert False, "unhandled option"
+
+    if len(args) == 0:
+        raise InternalShellError(cmd, "Error: 'rm' is missing an operand")
+
+    def on_rm_error(func, path, exc_info):
+        # path contains the path of the file that couldn't be removed
+        # let's just assume that it's read-only and remove it.
+        os.chmod(path, stat.S_IMODE( os.stat(path).st_mode) | stat.S_IWRITE)
+        os.remove(path)
+
+    for dir in args:
+        if not os.path.isabs(dir):
+            dir = os.path.realpath(os.path.join(cmd_shenv.cwd, dir))
+        if force and not os.path.exists(dir):
+            continue
+        try:
+            if os.path.isdir(dir):
+                if not recursive:
+                    raise InternalShellError(cmd, "Error: "
+                                                  "%s is a directory" % dir)
+                shutil.rmtree(dir, onerror = on_rm_error if force else None)
+            else:
+                if force and not os.access(dir, os.W_OK):
+                    os.chmod(dir,
+                             stat.S_IMODE(os.stat(dir).st_mode) | stat.S_IWRITE)
+                os.remove(dir)
+        except OSError as e:
+            raise InternalShellError(cmd, "Error: 'rm' command failed")
+
 def processRedirects(cmd, stdin_source, cmd_shenv, opened_files):
     """Return the standard fds for cmd after applying redirects
 
     Returns the three standard file descriptors for the new child process.  Each
     fd may be an open, writable file object or a sentinel value from the
     subprocess module.
     """
 
def _executeShCmd(cmd, shenv, results, timeoutHelper):
     if cmd.commands[0].args[0] == 'export':
         if len(cmd.commands) != 1:
             raise ValueError("'export' cannot be part of a pipeline")
         if len(cmd.commands[0].args) != 2:
             raise ValueError("'export' supports only one argument")
         updateEnv(shenv, cmd.commands[0])
         return 0
 
+    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")
+        executeBuiltinMkdir(cmd.commands[0], shenv)
+        return 0
+
+    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")
+        executeBuiltinDiff(cmd.commands[0], shenv)
+        return 0
+
+    if cmd.commands[0].args[0] == 'rm':
+        if len(cmd.commands) != 1:
+            raise InternalShellError(cmd.commands[0], "Unsupported: 'rm' "
+                                     "cannot be part of a pipeline")
+        executeBuiltinRm(cmd.commands[0], shenv)
+        return 0
+
     procs = []
     default_stdin = subprocess.PIPE
     stderrTempFiles = []
     opened_files = []
     named_temp_files = []
     # To avoid deadlock, we use a single stderr stream for piped
     # output. This is null until we have seen some output using
     # stderr.

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/check_path.py

+#!/usr/bin/env python
+
+import os
+import sys
+
+
+def check_path(argv):
+    if len(argv) < 3:
+        print("Wrong number of args")
+        return 1
+
+    type = argv[1]
+    paths = argv[2:]
+    exit_code = 0
+
+    if type == 'dir':
+        for idx, dir in enumerate(paths):
+            print(os.path.isdir(dir))
+    elif type == 'file':
+        for idx, file in enumerate(paths):
+            print(os.path.isfile(file))
+    else:
+        print("Unrecognised type {}".format(type))
+        exit_code = 1
+    return exit_code
+
+if __name__ == '__main__':
+    sys.exit (check_path (sys.argv))

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/diff-error-0.txt

+# Check error on a unsupported diff (cannot be part of a pipeline).
+#
+# RUN: diff diff-error-0.txt diff-error-0.txt | echo Output

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/diff-error-1.txt

+# Check error on a unsupported diff (not support the -B option).
+#
+# RUN: diff -B temp1.txt temp2.txt

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/diff-error-2.txt

+# Check error on an internal shell error (missing tofile)
+#
+# RUN: diff temp.txt

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/diff-error-3.txt

+# Check error on an internal shell error (unable to find compared files)
+#
+# RUN: diff temp.txt temp1.txt

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/diff-error-4.txt

+# Check error on an internal shell error (file's contexts are different)
+#
+# RUN: echo "hello-first" > %t
+# RUN: echo "hello-second" > %t1
+# RUN: diff %t %t1

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/diff-error-5.txt

+# Check error on an internal shell error (missing operand)
+#
+# RUN: diff -u

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/diff-error-6.txt

+# Check error on an internal shell error (extra operand)
+#
+# RUN: diff -u a.txt b.txt c.txt

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/mkdir-error-0.txt

+# Check error on a unsupported mkdir (cannot be part of a pipeline).
+#
+# RUN: mkdir -p temp | rm -rf temp

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/mkdir-error-1.txt

+# Check error on a unsupported mkdir (only does not support -m option).
+#
+# RUN: mkdir -p -m 777 temp

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/mkdir-error-2.txt

+# Check error on a unsupported mkdir (missing operand).
+#
+# RUN: mkdir -p

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/rm-error-0.txt

+# Check error on a unsupported rm. (cannot be part of a pipeline)
+#
+# RUN: rm -rf temp | echo "hello"

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/rm-error-1.txt

+# Check error on a unsupported rm (only does not support -v option).
+#
+# RUN: rm -f -v temp

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/rm-error-2.txt

+# Check error on a unsupported rm (only supports -f option and in combination with -r).
+#
+# RUN: rm -r hello

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/rm-error-3.txt

+# Check error on a unsupported rm (can't remove test since it is a directory).
+#
+# RUN: mkdir -p test
+# RUN: rm -f test

llvm/trunk/utils/lit/tests/Inputs/shtest-shell/valid-shell.txt

+# Check rm file operations.
+# Check force remove commands success whether the file does or doesn't exist.
+#
+# RUN: rm -f %t.write
+# RUN: "%{python}" %S/check_path.py file %t.write > %t.out
+# RUN: FileCheck --check-prefix=REMOVE-FILE < %t.out %s
+# RUN: echo "create a temp file" > %t.write
+# RUN: "%{python}" %S/check_path.py file %t.write > %t.out
+# RUN: FileCheck --check-prefix=FILE-EXIST < %t.out %s
+# RUN: rm -f %t.write
+# RUN: "%{python}" %S/check_path.py file %t.write > %t.out
+# RUN: FileCheck --check-prefix=REMOVE-FILE < %t.out %s
+#
+# REMOVE-FILE: False
+# FILE-EXIST: True
+#
+# Check mkdir and rm folder operations.
+# Check force remove commands success whether the directory does or doesn't exist.
+#
+# Check the mkdir command with -p option.
+# RUN: rm -f -r %T/test
+# RUN: "%{python}" %S/check_path.py dir %T/test > %t.out
+# RUN: FileCheck --check-prefix=REMOVE-PARENT-DIR < %t.out %s
+# RUN: mkdir -p %T/test
+# RUN: "%{python}" %S/check_path.py dir %T/test > %t.out
+# RUN: FileCheck --check-prefix=MAKE-PARENT-DIR < %t.out %s
+# RUN: rm -f -r %T/test
+# RUN: "%{python}" %S/check_path.py dir %T/test > %t.out
+# RUN: FileCheck --check-prefix=REMOVE-PARENT-DIR < %t.out %s
+#
+# MAKE-PARENT-DIR: True
+# REMOVE-PARENT-DIR: False
+#
+# Check the mkdir command without -p option.
+#
+# RUN: rm -rf %T/test1
+# RUN: mkdir %T/test1
+# RUN: "%{python}" %S/check_path.py dir %T/test1 > %t.out
+# RUN: FileCheck --check-prefix=MAKE-DIR < %t.out %s
+# RUN: cd %T/test1 && mkdir foo
+# RUN: "%{python}" %S/check_path.py dir %T/test1 > %t.out
+# RUN: FileCheck --check-prefix=MAKE-DIR < %t.out %s
+# RUN: cd %T && rm -rf %T/test1
+# RUN: "%{python}" %S/check_path.py dir %T/test1 > %t.out
+# RUN: FileCheck --check-prefix=REMOVE-DIR < %t.out %s
+#
+# MAKE-DIR: True
+# REMOVE-DIR: False
+#
+# Check creating and removing multiple folders and rm * operation.
+#
+# RUN: rm -rf %T/test
+# RUN: mkdir -p %T/test/test1 %T/test/test2
+# RUN: "%{python}" %S/check_path.py dir %T/test %T/test/test1 %T/test/test2 > %t.out
+# RUN: FileCheck --check-prefix=DIRS-EXIST < %t.out %s
+# RUN: echo "create a temp file" > %T/test/temp.write
+# RUN: echo "create a temp1 file" > %T/test/test1/temp1.write
+# RUN: echo "create a temp2 file" > %T/test/test2/temp2.write
+# RUN: "%{python}" %S/check_path.py file %T/test/temp.write %T/test/test1/temp1.write %T/test/test2/temp2.write> %t.out
+# RUN: FileCheck --check-prefix=FILES-EXIST < %t.out %s
+# RUN: rm -r -f %T/*
+# RUN: "%{python}" %S/check_path.py dir %T/test > %t.out
+# RUN: FileCheck --check-prefix=REMOVE-ALL < %t.out %s
+#
+# DIRS-EXIST: True
+# DIRS-EXIST-NEXT: True
+# DIRS-EXIST-NEXT: True
+# FILES-EXIST: True
+# FILES-EXIST-NEXT: True
+# FILES-EXIST-NEXT: True
+# REMOVE-ALL: False
+#
+# Check diff operations.
+#
+# RUN: echo "hello" > %t.stdout
+# RUN: echo "hello" > %t1.stdout
+# RUN: diff %t.stdout %t1.stdout
+# RUN: diff -u %t.stdout %t1.stdout
+#
+# RUN: mkdir -p %T/dir1 %T/dir2
+# RUN: cd %T/dir1 && echo "hello" > temp1.txt
+# RUN: cd %T/dir2 && echo "hello" > temp2.txt
+# RUN: diff temp2.txt ../dir1/temp1.txt

llvm/trunk/utils/lit/tests/max-failures.py

 # Check the behavior of --max-failures option.
 #
 # RUN: not %{lit} -j 1 -v %{inputs}/max-failures > %t.out
 # RUN: not %{lit} --max-failures=1 -j 1 -v %{inputs}/max-failures >> %t.out
 # RUN: not %{lit} --max-failures=2 -j 1 -v %{inputs}/max-failures >> %t.out
 # RUN: not %{lit} --max-failures=0 -j 1 -v %{inputs}/max-failures 2>> %t.out
 # RUN: FileCheck < %t.out %s
 #
 # END.
 
-# CHECK: Failing Tests (3)
+# CHECK: Failing Tests (17)
 # CHECK: Failing Tests (1)
 # CHECK: Failing Tests (2)
 # CHECK: error: Setting --max-failures to 0 does not have any effect.

llvm/trunk/utils/lit/tests/shtest-shell.py

 # Check the internal shell handling component of the ShTest format.
 #
 # RUN: not %{lit} -j 1 -v %{inputs}/shtest-shell > %t.out
 # FIXME: Temporarily dump test output so we can debug failing tests on
 # buildbots.
 # RUN: cat %t.out
 # RUN: FileCheck --input-file %t.out %s
 #
 # END.
 
 # CHECK: -- Testing:
 
+
+# CHECK: FAIL: shtest-shell :: diff-error-0.txt
+# CHECK: *** TEST 'shtest-shell :: diff-error-0.txt' FAILED ***
+# CHECK: $ "diff" "diff-error-0.txt" "diff-error-0.txt"
+# CHECK: # command stderr:
+# CHECK: Unsupported: 'diff' cannot be part of a pipeline
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: diff-error-1.txt
+# CHECK: *** TEST 'shtest-shell :: diff-error-1.txt' FAILED ***
+# CHECK: $ "diff" "-B" "temp1.txt" "temp2.txt"
+# CHECK: # command stderr:
+# CHECK: Unsupported: 'diff': option -B not recognized
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: diff-error-2.txt
+# CHECK: *** TEST 'shtest-shell :: diff-error-2.txt' FAILED ***
+# CHECK: $ "diff" "temp.txt"
+# CHECK: # command stderr:
+# CHECK: Error:  missing or extra operand
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: diff-error-3.txt
+# CHECK: *** TEST 'shtest-shell :: diff-error-3.txt' FAILED ***
+# CHECK: $ "diff" "temp.txt" "temp1.txt"
+# CHECK: # command stderr:
+# CHECK: Error: 'diff' command failed
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: diff-error-4.txt
+# CHECK: *** TEST 'shtest-shell :: diff-error-4.txt' FAILED ***
+# CHECK: $ "diff"
+# CHECK: # command stderr:
+# CHECK: Error: The contents of fromfile and tofile are different
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: diff-error-5.txt
+# CHECK: *** TEST 'shtest-shell :: diff-error-5.txt' FAILED ***
+# CHECK: $ "diff"
+# CHECK: # command stderr:
+# CHECK: Error:  missing or extra operand
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: diff-error-6.txt
+# CHECK: *** TEST 'shtest-shell :: diff-error-6.txt' FAILED ***
+# CHECK: $ "diff"
+# CHECK: # command stderr:
+# CHECK: Error:  missing or extra operand
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
 # CHECK: FAIL: shtest-shell :: error-0.txt
 # CHECK: *** TEST 'shtest-shell :: error-0.txt' FAILED ***
 # CHECK: $ "not-a-real-command"
 # CHECK: # command stderr:
 # CHECK: 'not-a-real-command': command not found
 # CHECK: error: command failed with exit status: 127
 # CHECK: ***
 
 # FIXME: The output here sucks.
 #
 # CHECK: FAIL: shtest-shell :: error-1.txt
 # CHECK: *** TEST 'shtest-shell :: error-1.txt' FAILED ***
 # CHECK: shell parser error on: 'echo "missing quote'
 # CHECK: ***
 
 # CHECK: FAIL: shtest-shell :: error-2.txt
 # CHECK: *** TEST 'shtest-shell :: error-2.txt' FAILED ***
 # CHECK: Unsupported redirect:
 # CHECK: ***
 
+# CHECK: FAIL: shtest-shell :: mkdir-error-0.txt
+# CHECK: *** TEST 'shtest-shell :: mkdir-error-0.txt' FAILED ***
+# CHECK: $ "mkdir" "-p" "temp"
+# CHECK: # command stderr:
+# CHECK: Unsupported: 'mkdir' cannot be part of a pipeline
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: mkdir-error-1.txt
+# CHECK: *** TEST 'shtest-shell :: mkdir-error-1.txt' FAILED ***
+# CHECK: $ "mkdir" "-p" "-m" "777" "temp"
+# CHECK: # command stderr:
+# CHECK: Unsupported: 'mkdir': option -m not recognized
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: mkdir-error-2.txt
+# CHECK: *** TEST 'shtest-shell :: mkdir-error-2.txt' FAILED ***
+# CHECK: $ "mkdir" "-p"
+# CHECK: # command stderr:
+# CHECK: Error: 'mkdir' is missing an operand
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
 # CHECK: PASS: shtest-shell :: redirects.txt
+
+# CHECK: FAIL: shtest-shell :: rm-error-0.txt
+# CHECK: *** TEST 'shtest-shell :: rm-error-0.txt' FAILED ***
+# CHECK: $ "rm" "-rf" "temp"
+# CHECK: # command stderr:
+# CHECK: Unsupported: 'rm' cannot be part of a pipeline
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: rm-error-1.txt
+# CHECK: *** TEST 'shtest-shell :: rm-error-1.txt' FAILED ***
+# CHECK: $ "rm" "-f" "-v" "temp"
+# CHECK: # command stderr:
+# CHECK: Unsupported: 'rm': option -v not recognized
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: rm-error-2.txt
+# CHECK: *** TEST 'shtest-shell :: rm-error-2.txt' FAILED ***
+# CHECK: $ "rm" "-r" "hello"
+# CHECK: # command stderr:
+# CHECK: Error: 'rm' command failed
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
+# CHECK: FAIL: shtest-shell :: rm-error-3.txt
+# CHECK: *** TEST 'shtest-shell :: rm-error-3.txt' FAILED ***
+# CHECK: $ "rm" "-f" "test"
+# CHECK: # command stderr:
+# CHECK: test is a directory
+# CHECK: error: command failed with exit status: 127
+# CHECK: ***
+
 # CHECK: PASS: shtest-shell :: sequencing-0.txt
 # CHECK: XFAIL: shtest-shell :: sequencing-1.txt
-# CHECK: Failing Tests (3)
+# CHECK: PASS: shtest-shell :: valid-shell.txt
+# CHECK: Failing Tests (17)