[internal] error in CI if tests skipped (pantsbuild#14994)

* error in CI if tests skipped

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

* Switch to providing our own infra + mark

* root conftest

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

* fixy

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

* Move to ci toml

* no brackets

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

* maybe this

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

* fmt

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

* fix integration test

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

* exist ok

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]

BUILD

@@ -6,3 +6,5 @@ shell_sources(name="scripts", sources=["cargo", "pants"])
 # We use `BUILD_ROOT` to establish the build root, rather than `./pants`, per
 # https://github.com/pantsbuild/pants/pull/8105.
 files(name="files", sources=["BUILD_ROOT", ".gitignore", "pants.toml"])
+
+python_test_utils(name="test_utils")

conftest.py

@@ -0,0 +1,33 @@
+# Copyright 2022 Pants project contributors (see CONTRIBUTORS.md).
+# Licensed under the Apache License, Version 2.0 (see LICENSE).
+
+import pytest
+
+
+@pytest.hookimpl(hookwrapper=True)
+def pytest_runtest_makereport(item: pytest.Item, call: pytest.CallInfo[None]):
+    outcome = yield
+    rep = outcome.get_result()
+
+    if (
+        item.config.getoption("--noskip")
+        and rep.skipped
+        and (call.excinfo and call.excinfo.errisinstance(pytest.skip.Exception))
+        and "no_error_if_skipped" not in item.keywords
+    ):
+        rep.outcome = "failed"
+        assert call.excinfo is not None
+        r = call.excinfo._getreprcrash()
+        rep.longrepr = f"Forbidden skipped test - {r.message}"
+
+
+def pytest_configure(config):
+    config.addinivalue_line(
+        "markers", "no_error_if_skipped: Don't error if this test is skipped when using --noskip"
+    )
+
+
+def pytest_addoption(parser):
+    parser.addoption(
+        "--noskip", action="store_true", default=False, help="Treat skipped tests as errors"
+    )

pants.ci.toml

@@ -15,7 +15,7 @@ use_coverage = true
 xml_dir = "dist/test-results/"
 
 [pytest]
-args = ["--no-header", "-vv"]
+args = ["--no-header", "--noskip", "-vv"]
 
 [coverage-py]
 report = ["raw", "xml"]
@@ -30,7 +30,7 @@ ci_env_variables = [
   "GITHUB_WORKFLOW","GITHUB_JOB",  # temporary, for debugging issues w/ restricted tokens.
 ]
 restricted_token_matches =  """{
-  'GITHUB_REPOSITORY': 'pantsbuild/pants', 
+  'GITHUB_REPOSITORY': 'pantsbuild/pants',
 }"""
 
 [buildsense]

pants.toml

@@ -91,6 +91,8 @@ root_patterns = [
   "/build-support/flake8",
   "/build-support/migration-support",
   "/pants-plugins",
+  # For `conftest.py`
+  "/",
 ]
 
 [tailor]

src/python/pants/backend/go/goals/test_test.py

@@ -507,6 +507,7 @@ def test_both_internal_and_external_tests_fail(rule_runner: RuleRunner) -> None:
     assert "FAIL: TestAddExternal" in result.stdout
 
 
+@pytest.mark.no_error_if_skipped
 def test_fuzz_target_supported(rule_runner: RuleRunner) -> None:
     go_version_result = rule_runner.request(
         ProcessResult, [GoSdkProcess(["version"], description="Get `go` version.")]

src/python/pants/backend/shell/shunit2_test_runner_integration_test.py

@@ -191,6 +191,7 @@ def test_subdirectories(rule_runner: RuleRunner) -> None:
     "The Process is not being cached, but the rule invocation is being memoized so the "
     "`--force` does not work properly."
 )
+@pytest.mark.no_error_if_skipped
 def test_force(rule_runner: RuleRunner) -> None:
     rule_runner.write_files({"tests.sh": GOOD_TEST, "BUILD": "shunit2_tests(name='t')"})
     tgt = rule_runner.get_target(Address("", target_name="t", relative_file_path="tests.sh"))

src/python/pants/core/goals/publish_test.py

@@ -188,6 +188,7 @@ def test_structured_output(rule_runner: RuleRunner) -> None:
 
 
 @pytest.mark.skip("Can not run interactive process from test..?")
+@pytest.mark.no_error_if_skipped
 def test_mocked_publish(rule_runner: RuleRunner) -> None:
     rule_runner.write_files(
         {

src/python/pants/core/goals/run_integration_test.py

@@ -14,32 +14,35 @@
 
 @ensure_daemon
 def test_run_then_edit(use_pantsd: bool) -> None:
-    slow = "slow.py"
+    # These files must exist outside of a Pants `source_root` so that `coverage-py` doesn't try
+    # to collect coverage metrics for them (as they are local to the chroot and coverage will
+    # error unable to find their source)
+    dirname = "not-a-source-root"
     files = {
-        slow: dedent(
+        f"{dirname}/slow.py": dedent(
             """\
-        import time
-        time.sleep(30)
-        raise Exception("Should have been restarted by now!")
-        """
+            import time
+            time.sleep(30)
+            raise Exception("Should have been restarted by now!")
+            """
         ),
-        "BUILD": dedent(
-            f"""\
-        python_sources(name='lib')
-        pex_binary(name='bin', entry_point='{slow}', restartable=True)
-        """
+        f"{dirname}/BUILD": dedent(
+            """\
+            python_sources(name='lib')
+            pex_binary(name='bin', entry_point='slow.py', restartable=True)
+            """
         ),
     }
+    Path(dirname).mkdir(exist_ok=True)
     for name, content in files.items():
         Path(name).write_text(content)
 
     with temporary_workdir() as workdir:
-
         client_handle = run_pants_with_workdir_without_waiting(
             [
                 "--backend-packages=['pants.backend.python']",
                 "run",
-                slow,
+                f"{dirname}/slow.py",
             ],
             workdir=workdir,
             use_pantsd=use_pantsd,
@@ -50,7 +53,7 @@ def test_run_then_edit(use_pantsd: bool) -> None:
         assert client_handle.process.poll() is None
 
         # Edit the file to restart the run, and check that it re-ran
-        Path(slow).write_text('print("No longer slow!")')
+        Path(f"{dirname}/slow.py").write_text('print("No longer slow!")')
         result = client_handle.join()
         result.assert_success()
         assert result.stdout == "No longer slow!\n"

src/python/pants/engine/rules_test.py

@@ -587,6 +587,7 @@ def a_from_b(b: B) -> A:
         ) in str(cm.value)
 
     @pytest.mark.skip(reason="TODO(#10649): figure out if this tests is still relevant.")
+    @pytest.mark.no_error_if_skipped
     def test_not_fulfillable_duplicated_dependency(self) -> None:
         # If a rule depends on another rule+subject in two ways, and one of them is unfulfillable
         # Only the unfulfillable one should be in the errors.
@@ -631,6 +632,7 @@ async def d_from_a_and_suba(a: A, suba: SubA) -> D:  # type: ignore[return]
     @pytest.mark.skip(
         reason="TODO(#10649): Fix and re-enable once reachability checks are restored."
     )
+    @pytest.mark.no_error_if_skipped
     def test_unreachable_rule(self) -> None:
         """Test that when one rule "shadows" another, we get an error."""
 
@@ -827,6 +829,7 @@ def b_from_suba(suba: SubA) -> B:
         )
 
     @pytest.mark.skip(reason="TODO(#10649): figure out if this tests is still relevant.")
+    @pytest.mark.no_error_if_skipped
     def test_noop_removal_in_subgraph(self) -> None:
         @rule
         def a_from_c(c: C) -> A:
@@ -859,6 +862,7 @@ def b_singleton() -> B:
         )
 
     @pytest.mark.skip(reason="TODO(#10649): figure out if this tests is still relevant.")
+    @pytest.mark.no_error_if_skipped
     def test_noop_removal_full_single_subject_type(self) -> None:
         @rule
         def a_from_c(c: C) -> A:
@@ -887,6 +891,7 @@ def a() -> A:
         )
 
     @pytest.mark.skip(reason="TODO(#10649): figure out if this tests is still relevant.")
+    @pytest.mark.no_error_if_skipped
     def test_root_tuple_removed_when_no_matches(self) -> None:
         @rule
         def a_from_c(c: C) -> A:
@@ -922,6 +927,7 @@ def b_from_d_and_a(d: D, a: A) -> B:
         )
 
     @pytest.mark.skip(reason="TODO(#10649): figure out if this tests is still relevant.")
+    @pytest.mark.no_error_if_skipped
     def test_noop_removal_transitive(self) -> None:
         # If a noop-able rule has rules that depend on it,
         # they should be removed from the graph.
@@ -990,6 +996,7 @@ def b_singleton() -> B:
         )
 
     @pytest.mark.skip(reason="TODO(#10649): figure out if this tests is still relevant.")
+    @pytest.mark.no_error_if_skipped
     def test_depends_on_multiple_one_noop(self) -> None:
         @rule
         def a_from_c(c: C) -> A:
@@ -1065,6 +1072,7 @@ def c_from_a(a: A) -> C:
         )
 
     @pytest.mark.skip(reason="TODO(#10649): figure out if this tests is still relevant.")
+    @pytest.mark.no_error_if_skipped
     def test_get_simple(self) -> None:
         @rule
         async def a() -> A:  # type: ignore[return]

src/python/pants/jvm/jdk_rules_test.py

@@ -81,6 +81,7 @@ def test_java_binary_bogus_version_fails(rule_runner: RuleRunner) -> None:
 
 @maybe_skip_jdk_test
 @pytest.mark.skip(reason="#12293 Coursier JDK bootstrapping is currently flaky in CI")
+@pytest.mark.no_error_if_skipped
 def test_java_binary_versions(rule_runner: RuleRunner) -> None:
     # default version is 1.11
     assert "javac 11.0" in run_javac_version(rule_runner)

src/python/pants/jvm/test/junit_test.py

@@ -696,6 +696,7 @@ def test_vintage_files_dependencies(rule_runner: RuleRunner) -> None:
 
 
 @pytest.mark.skip  # TODO(14537) `relocated_files` doesn't presently work, un-skip when fixing that.
+@pytest.mark.no_error_if_skipped
 @maybe_skip_jdk_test
 def test_vintage_relocated_files_dependency(rule_runner: RuleRunner) -> None:
     _write_file_dependencies(rule_runner, [":relocated_ducks"], "ducks/ducks.txt")

src/python/pants/testutil/pants_integration_test.py

@@ -114,6 +114,9 @@ def run_pants_with_workdir_without_waiting(
 
     if hermetic:
         args.append("--pants-config-files=[]")
+        # Certain tests may be invoking `./pants test` for a pytest test with conftest discovery
+        # enabled. We should ignore the root conftest.py for these cases.
+        args.append("--pants-ignore=/conftest.py")
 
     if config:
         toml_file_name = os.path.join(workdir, "pants.toml")

src/python/pants/vcs/changed_integration_test.py

@@ -353,6 +353,7 @@ def test_changed_with_deleted_resource(self):
             assert pants_run.stdout.strip() == "src/python/sources:text"
 
     @pytest.mark.skip(reason="Unskip after rewriting these tests to stop using testprojects.")
+    @pytest.mark.no_error_if_skipped
     def test_changed_with_deleted_target_transitive(self):
         # TODO: The deleted target should be a dependee of another target. We want to make sure
         # that this causes a crash because the dependee can't find it's dependency.

tests/python/pants_test/integration/goal_rule_integration_test.py

@@ -29,6 +29,7 @@ def test_unimplemented_goals_noop() -> None:
 
 
 @pytest.mark.skip(reason="Flaky test. https://github.com/pantsbuild/pants/issues/10478")
+@pytest.mark.no_error_if_skipped
 class TestGoalRuleIntegration(PantsDaemonIntegrationTestBase):
     hermetic = False
 

tests/python/pants_test/integration/graph_integration_test.py

@@ -8,6 +8,8 @@
 from textwrap import dedent
 from typing import Iterator
 
+import pytest
+
 from pants.option.scope import GLOBAL_SCOPE_CONFIG_SECTION
 from pants.testutil.pants_integration_test import run_pants
 from pants.util.contextutil import overwrite_file_content
@@ -73,6 +75,7 @@ def setup_sources_targets() -> Iterator[None]:
 
 
 @unittest.skip("flaky: https://github.com/pantsbuild/pants/issues/8520")
+@pytest.mark.no_error_if_skipped
 def test_missing_sources_warnings():
     target_to_unmatched_globs = {
         "missing-globs": ["*.a"],
@@ -104,6 +107,7 @@ def test_missing_sources_warnings():
 
 
 @unittest.skip("flaky: https://github.com/pantsbuild/pants/issues/8520")
+@pytest.mark.no_error_if_skipped
 def test_existing_sources():
     target_full = f"{_SOURCES_TARGET_BASE}:text"
     pants_run = run_pants(
@@ -124,6 +128,7 @@ def test_existing_directory_with_no_build_files_fails():
 
 
 @unittest.skip("flaky: https://github.com/pantsbuild/pants/issues/6787")
+@pytest.mark.no_error_if_skipped
 def test_error_message():
     with setup_sources_targets():
         for target in _ERR_TARGETS:

tests/python/pants_test/pantsd/pantsd_integration_test.py

@@ -267,6 +267,7 @@ def test_pantsd_aligned_output(self) -> None:
                 assert line_pair[0] == line_pair[1]
 
     @unittest.skip("flaky: https://github.com/pantsbuild/pants/issues/7622")
+    @pytest.mark.no_error_if_skipped
     def test_pantsd_filesystem_invalidation(self):
         """Runs with pantsd enabled, in a loop, while another thread invalidates files."""
         with self.pantsd_successful_run_context() as ctx:
@@ -415,6 +416,7 @@ def test_pantsd_pid_change(self):
             os.unlink(pidpath)
 
     @pytest.mark.skip(reason="flaky: https://github.com/pantsbuild/pants/issues/8193")
+    @pytest.mark.no_error_if_skipped
     def test_pantsd_memory_usage(self):
         """Validates that after N runs, memory usage has increased by no more than X percent."""
         number_of_runs = 10

tests/python/pants_test/pantsd/test_process_manager.py

@@ -64,6 +64,7 @@ def test_readwrite_metadata_by_name(self):
             )
 
     @pytest.mark.skip(reason="flaky: https://github.com/pantsbuild/pants/issues/6836")
+    @pytest.mark.no_error_if_skipped
     def test_deadline_until(self):
         with self.assertRaises(ProcessManager.Timeout):
             with self.captured_logging(logging.INFO) as captured: