Remove capsys.disabled magic from CLI tests

The addition of capsys.disabled calls to the CLI tests was done to work around an issue with files being opened and closed in the CLI setup fixtures. When running under high parallelism the CLI tests would occasionally be split up across worker instances, and so the file system operations - which were effectively creating and removing the same file - would cause failures due to race conditions between the different test runner fixture executions. Adding capsys.disabled didn't solve this problem, it merely made it somewhat less likely to occur, and ultimately replaced the error with a different one (file not found). This commit removes the capsys.disabled calls in favor of a follow-up to stop doing all of this filesystem manipulation.
dbt-labs · Oct 31, 2023 · e62ca4c · e62ca4c
1 parent 5b6c138
commit e62ca4c
Showing 1 changed file with 45 additions and 68 deletions.
diff --git a/metricflow/test/cli/test_cli.py b/metricflow/test/cli/test_cli.py
@@ -37,38 +37,30 @@
 # TODO: Use snapshots to compare CLI output for all tests here.
 
 
-def test_query(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(query, args=["--metrics", "bookings", "--group-by", "metric_time"])
+def test_query(cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
+    resp = cli_runner.run(query, args=["--metrics", "bookings", "--group-by", "metric_time"])
     # case insensitive matches are needed for snowflake due to the capitalization thing
     engine_is_snowflake = cli_runner.cli_context.sql_client.sql_engine_type is SqlEngine.SNOWFLAKE
     assert "bookings" in resp.output or ("bookings" in resp.output.lower() and engine_is_snowflake)
     assert resp.exit_code == 0
 
 
-def test_list_dimensions(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(dimensions, args=["--metrics", "bookings"])
+def test_list_dimensions(cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
+    resp = cli_runner.run(dimensions, args=["--metrics", "bookings"])
 
     assert "ds" in resp.output
     assert resp.exit_code == 0
 
 
-def test_list_metrics(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(metrics)
+def test_list_metrics(cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
+    resp = cli_runner.run(metrics)
 
     assert "bookings_per_listing: listing__capacity_latest" in resp.output
     assert resp.exit_code == 0
 
 
-def test_get_dimension_values(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(dimension_values, args=["--metrics", "bookings", "--dimension", "booking__is_instant"])
+def test_get_dimension_values(cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
+    resp = cli_runner.run(dimension_values, args=["--metrics", "bookings", "--dimension", "booking__is_instant"])
 
     actual_output_lines = sorted(resp.output.split("\n"))
     assert ["", "• False", "• True"] == actual_output_lines
@@ -84,7 +76,7 @@ def create_directory(directory_path: str) -> Iterator[None]:
     shutil.rmtree(path)
 
 
-def test_validate_configs(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
+def test_validate_configs(cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
     yaml_contents = textwrap.dedent(
         """\
         semantic_model:
@@ -111,24 +103,20 @@ def test_validate_configs(capsys: pytest.CaptureFixture, cli_runner: MetricFlowC
         manifest_file = target_directory / "semantic_manifest.json"
         manifest_file.write_text(manifest.json())
 
-        # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-        with capsys.disabled():
-            resp = cli_runner.run(validate_configs)
+        resp = cli_runner.run(validate_configs)
 
         assert "ERROR" in resp.output
         assert resp.exit_code == 0
 
 
-def test_health_checks(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(health_checks)
+def test_health_checks(cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
+    resp = cli_runner.run(health_checks)
 
     assert "SELECT 1: Success!" in resp.output
     assert resp.exit_code == 0
 
 
-def test_tutorial_message(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:
+def test_tutorial_message(cli_runner: MetricFlowCliRunner) -> None:
     """Tests the message output of the tutorial.
 
     The tutorial now essentially compiles a semantic manifest and then asks the user to run dbt seed,
@@ -139,17 +127,14 @@ def test_tutorial_message(capsys: pytest.CaptureFixture, cli_runner: MetricFlowC
     project path overrides it might warrant a more complete test of the semantic manifest building steps in the
     tutorial flow.
     """
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(tutorial, args=["-m"])
+    resp = cli_runner.run(tutorial, args=["-m"])
     assert "Please run the following steps" in resp.output
     assert "dbt seed" in resp.output
 
 
-def test_list_entities(capsys: pytest.CaptureFixture, cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
+def test_list_entities(cli_runner: MetricFlowCliRunner) -> None:  # noqa: D
     # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(entities, args=["--metrics", "bookings"])
+    resp = cli_runner.run(entities, args=["--metrics", "bookings"])
 
     assert "guest" in resp.output
     assert "host" in resp.output
@@ -163,11 +148,9 @@ def test_saved_query(  # noqa: D
     cli_runner: MetricFlowCliRunner,
     sql_client: SqlClient,
 ) -> None:
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(
-            query, args=["--saved-query", "p0_booking", "--order", "metric_time__day,listing__capacity_latest"]
-        )
+    resp = cli_runner.run(
+        query, args=["--saved-query", "p0_booking", "--order", "metric_time__day,listing__capacity_latest"]
+    )
 
     assert resp.exit_code == 0
 
@@ -188,19 +171,17 @@ def test_saved_query_with_where(  # noqa: D
     cli_runner: MetricFlowCliRunner,
     sql_client: SqlClient,
 ) -> None:
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(
-            query,
-            args=[
-                "--saved-query",
-                "p0_booking",
-                "--order",
-                "metric_time__day,listing__capacity_latest",
-                "--where",
-                "{{ Dimension('listing__capacity_latest') }} > 4",
-            ],
-        )
+    resp = cli_runner.run(
+        query,
+        args=[
+            "--saved-query",
+            "p0_booking",
+            "--order",
+            "metric_time__day,listing__capacity_latest",
+            "--where",
+            "{{ Dimension('listing__capacity_latest') }} > 4",
+        ],
+    )
 
     assert resp.exit_code == 0
 
@@ -221,19 +202,17 @@ def test_saved_query_with_limit(  # noqa: D
     cli_runner: MetricFlowCliRunner,
     sql_client: SqlClient,
 ) -> None:
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(
-            query,
-            args=[
-                "--saved-query",
-                "p0_booking",
-                "--order",
-                "metric_time__day,listing__capacity_latest",
-                "--limit",
-                "3",
-            ],
-        )
+    resp = cli_runner.run(
+        query,
+        args=[
+            "--saved-query",
+            "p0_booking",
+            "--order",
+            "metric_time__day,listing__capacity_latest",
+            "--limit",
+            "3",
+        ],
+    )
 
     assert resp.exit_code == 0
 
@@ -251,12 +230,10 @@ def test_saved_query_explain(  # noqa: D
     mf_test_session_state: MetricFlowTestSessionState,
     cli_runner: MetricFlowCliRunner,
 ) -> None:
-    # Disabling capsys to resolve error "ValueError: I/O operation on closed file". Better solution TBD.
-    with capsys.disabled():
-        resp = cli_runner.run(
-            query,
-            args=["--explain", "--saved-query", "p0_booking", "--order", "metric_time__day,listing__capacity_latest"],
-        )
+    resp = cli_runner.run(
+        query,
+        args=["--explain", "--saved-query", "p0_booking", "--order", "metric_time__day,listing__capacity_latest"],
+    )
 
     # Currently difficult to compare explain output due to randomly generated IDs.
     assert resp.exit_code == 0