Skip test_foreground_background + other minor cleanups

The test_foreground_background unit test has been marked
xfail for a while, meaning:
- Nobody looks at the results of the test
- It still runs every time

That test happens to hang frequently on some Apple M1 I have access to,
so here I mark it as skip.

Also went through other xfailing and skipped tests, and applied minor changes.

lib/spack/spack/test/cmd/commands.py

@@ -6,7 +6,6 @@
 import filecmp
 import os
 import shutil
-import subprocess
 
 import pytest
 
@@ -156,22 +155,6 @@ def test_update_with_header(tmpdir):
     commands("--update", str(update_file), "--header", str(filename))
 
 
-@pytest.mark.xfail
-def test_no_pipe_error():
-    """Make sure we don't see any pipe errors when piping output."""
-
-    proc = subprocess.Popen(
-        ["spack", "commands", "--format=rst"], stdout=subprocess.PIPE, stderr=subprocess.PIPE
-    )
-
-    # Call close() on stdout to cause a broken pipe
-    proc.stdout.close()
-    proc.wait()
-    stderr = proc.stderr.read().decode("utf-8")
-
-    assert "Broken pipe" not in stderr
-
-
 def test_bash_completion():
     """Test the bash completion writer."""
     out1 = commands("--format=bash")

lib/spack/spack/test/cmd/external.py

@@ -100,7 +100,7 @@ def test_get_executables(working_env, mock_executable):
 
 # TODO: this test should be made to work, but in the meantime it is
 # causing intermittent (spurious) CI failures on all PRs
-@pytest.mark.skipif(sys.platform == "win32", reason="Test fails intermittently on Windows")
+@pytest.mark.not_on_windows("Test fails intermittently on Windows")
 def test_find_external_cmd_not_buildable(mutable_config, working_env, mock_executable):
     """When the user invokes 'spack external find --not-buildable', the config
     for any package where Spack finds an external version should be marked as

lib/spack/spack/test/cmd/help.py

@@ -2,29 +2,13 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
-import pytest
-
 from spack.main import SpackCommand
 
 
-@pytest.mark.xfail
 def test_reuse_after_help():
     """Test `spack help` can be called twice with the same SpackCommand."""
     help_cmd = SpackCommand("help", subprocess=True)
     help_cmd()
-
-    # This second invocation will somehow fail because the parser no
-    # longer works after add_all_commands() is called in
-    # SpackArgumentParser.format_help_sections().
-    #
-    # TODO: figure out why this doesn't work properly and change this
-    # test to use a single SpackCommand.
-    #
-    # It seems that parse_known_args() finds "too few arguments" the
-    # second time through b/c add_all_commands() ends up leaving extra
-    # positionals in the parser. But this used to work before we loaded
-    # commands lazily.
     help_cmd()
 
 

lib/spack/spack/test/cmd/style.py

@@ -24,6 +24,12 @@
 style = spack.main.SpackCommand("style")
 
 
+ISORT = which("isort")
+BLACK = which("black")
+FLAKE8 = which("flake8")
+MYPY = which("mypy")
+
+
 @pytest.fixture(autouse=True)
 def has_develop_branch(git):
     """spack style requires git and a develop branch to run -- skip if we're missing either."""
@@ -190,8 +196,8 @@ def external_style_root(git, flake8_package_with_errors, tmpdir):
     yield tmpdir, py_file
 
 
-@pytest.mark.skipif(not which("isort"), reason="isort is not installed.")
+@pytest.mark.skipif(not ISORT, reason="isort is not installed.")
-@pytest.mark.skipif(not which("black"), reason="black is not installed.")
+@pytest.mark.skipif(not BLACK, reason="black is not installed.")
 def test_fix_style(external_style_root):
     """Make sure spack style --fix works."""
     tmpdir, py_file = external_style_root
@@ -209,10 +215,10 @@ def test_fix_style(external_style_root):
     assert filecmp.cmp(broken_py, fixed_py)
 
 
-@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+@pytest.mark.skipif(not FLAKE8, reason="flake8 is not installed.")
-@pytest.mark.skipif(not which("isort"), reason="isort is not installed.")
+@pytest.mark.skipif(not ISORT, reason="isort is not installed.")
-@pytest.mark.skipif(not which("mypy"), reason="mypy is not installed.")
+@pytest.mark.skipif(not MYPY, reason="mypy is not installed.")
-@pytest.mark.skipif(not which("black"), reason="black is not installed.")
+@pytest.mark.skipif(not BLACK, reason="black is not installed.")
 def test_external_root(external_style_root, capfd):
     """Ensure we can run in a separate root directory w/o configuration files."""
     tmpdir, py_file = external_style_root
@@ -238,7 +244,7 @@ def test_external_root(external_style_root, capfd):
     assert "lib/spack/spack/dummy.py:7: [F401] 'os' imported but unused" in output
 
 
-@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+@pytest.mark.skipif(not FLAKE8, reason="flake8 is not installed.")
 def test_style(flake8_package, tmpdir):
     root_relative = os.path.relpath(flake8_package, spack.paths.prefix)
 
@@ -264,7 +270,7 @@ def test_style(flake8_package, tmpdir):
     assert "spack style checks were clean" in output
 
 
-@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+@pytest.mark.skipif(not FLAKE8, reason="flake8 is not installed.")
 def test_style_with_errors(flake8_package_with_errors):
     root_relative = os.path.relpath(flake8_package_with_errors, spack.paths.prefix)
     output = style(
@@ -275,8 +281,8 @@ def test_style_with_errors(flake8_package_with_errors):
     assert "spack style found errors" in output
 
 
-@pytest.mark.skipif(not which("black"), reason="black is not installed.")
+@pytest.mark.skipif(not BLACK, reason="black is not installed.")
-@pytest.mark.skipif(not which("flake8"), reason="flake8 is not installed.")
+@pytest.mark.skipif(not FLAKE8, reason="flake8 is not installed.")
 def test_style_with_black(flake8_package_with_errors):
     output = style("--tool", "black,flake8", flake8_package_with_errors, fail_on_error=False)
     assert "black found errors" in output

lib/spack/spack/test/container/cli.py

@@ -27,7 +27,7 @@ def test_listing_possible_os():
         assert expected_os in output
 
 
-@pytest.mark.skipif(str(spack.platforms.host()) == "windows", reason="test unsupported on Windows")
+@pytest.mark.not_on_windows("test unsupported on Windows")
 @pytest.mark.maybeslow
 @pytest.mark.requires_executables("git")
 def test_bootstrap_phase(minimal_configuration, config_dumper, capsys):

lib/spack/spack/test/installer.py

@@ -582,7 +582,7 @@ def test_clear_failures_success(tmpdir):
         assert os.path.isfile(failures.locker.lock_path)
 
 
-@pytest.mark.xfail(sys.platform == "win32", reason="chmod does not prevent removal on Win")
+@pytest.mark.not_on_windows("chmod does not prevent removal on Win")
 def test_clear_failures_errs(tmpdir, capsys):
     """Test the clear_failures exception paths."""
     failures = spack.database.FailureTracker(str(tmpdir), default_timeout=0.1)

lib/spack/spack/test/llnl/util/filesystem.py

@@ -274,7 +274,7 @@ def test_symlinks_false(self, stage):
                 assert not os.path.islink("dest/2")
             check_added_exe_permissions("source/2", "dest/2")
 
-    @pytest.mark.skipif(sys.platform == "win32", reason="Broken symlinks not allowed on Windows")
+    @pytest.mark.not_on_windows("Broken symlinks not allowed on Windows")
     def test_allow_broken_symlinks(self, stage):
         """Test installing with a broken symlink."""
         with fs.working_dir(str(stage)):

lib/spack/spack/test/llnl/util/tty/log.py

@@ -348,7 +348,7 @@ def no_termios():
         (mock_shell_tstp_tstp_cont_cont, no_termios),
     ],
 )
-@pytest.mark.xfail(reason="Fails almost consistently when run with coverage and xdist")
+@pytest.mark.skip(reason="Fails almost consistently when run with coverage and xdist")
 def test_foreground_background(test_fn, termios_on_or_off, tmpdir):
     """Functional tests for foregrounding and backgrounding a logged process.
 
@@ -465,7 +465,7 @@ def mock_shell_v_v_no_termios(proc, ctl, **kwargs):
     "test_fn,termios_on_or_off",
     [(mock_shell_v_v, lang.nullcontext), (mock_shell_v_v_no_termios, no_termios)],
 )
-@pytest.mark.xfail(reason="Fails almost consistently when run with coverage and xdist")
+@pytest.mark.skip(reason="Fails almost consistently when run with coverage and xdist")
 def test_foreground_background_output(test_fn, capfd, termios_on_or_off, tmpdir):
     """Tests hitting 'v' toggles output, and that force_echo works."""
     if sys.version_info >= (3, 8) and sys.platform == "darwin" and termios_on_or_off == no_termios:

lib/spack/spack/test/make_executable.py

@@ -9,16 +9,13 @@
 This just tests whether the right args are getting passed to make.
 """
 import os
-import sys
 
 import pytest
 
 from spack.build_environment import MakeExecutable
 from spack.util.environment import path_put_first
 
-pytestmark = pytest.mark.skipif(
+pytestmark = pytest.mark.not_on_windows("MakeExecutable not supported on Windows")
-    sys.platform == "win32", reason="MakeExecutable not supported on Windows"
-)
 
 
 @pytest.fixture(autouse=True)

lib/spack/spack/test/spack_yaml.py

@@ -2,10 +2,8 @@
 # Spack Project Developers. See the top-level COPYRIGHT file for details.
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
-
 """Test Spack's custom YAML format."""
 import io
-import sys
 
 import pytest
 
@@ -126,7 +124,7 @@ def test_yaml_aliases():
         ),
     ],
 )
-@pytest.mark.xfail(sys.platform == "win32", reason="fails on Windows")
+@pytest.mark.not_on_windows(reason="fails on Windows")
 def test_round_trip_configuration(initial_content, expected_final_content, tmp_path):
     """Test that configuration can be loaded and dumped without too many changes"""
     file = tmp_path / "test.yaml"