From 408e1b2167de7ca37547a9a9ef5108b047b5d127 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Mon, 27 Jul 2020 20:33:24 -0700 Subject: [PATCH 1/2] Improve test output. - Adds a flag that allows you to select whether test stdout/stderr is displayed for all tests, failed tests or no tests. Defaults to failed tests, which is the most useful setting. That way when running over many tests you can easily focus on just the failed ones. - Shows the summary in all cases. It's weird to have special-case different behavior when there's only one test target in play vs if there are two. [ci skip-rust] [ci skip-build-wheels] --- src/python/pants/core/goals/test.py | 41 ++++++++++++++----- src/python/pants/core/goals/test_test.py | 51 +++++++++++++++++++++++- 2 files changed, 80 insertions(+), 12 deletions(-) diff --git a/src/python/pants/core/goals/test.py b/src/python/pants/core/goals/test.py index dcd71a0757d..3c03a8b0810 100644 --- a/src/python/pants/core/goals/test.py +++ b/src/python/pants/core/goals/test.py @@ -200,6 +200,14 @@ def materialize(self, console: Console, workspace: Workspace) -> Tuple[PurePath, return tuple(report_paths) +class ShowOutput(Enum): + """Which tests to emit detailed output for.""" + + ALL = "all" + FAILED = "failed" + NONE = "none" + + class TestSubsystem(GoalSubsystem): """Run tests.""" @@ -228,6 +236,12 @@ def register_options(cls, register) -> None: default=False, help="Force the tests to run, even if they could be satisfied from cache.", ) + register( + "--show-output", + type=ShowOutput, + default=ShowOutput.FAILED, + help="Show stdout/stderr for these tests.", + ) register( "--use-coverage", type=bool, @@ -307,11 +321,13 @@ async def run_tests( for field_set in field_sets_with_sources ) - exit_code = PANTS_SUCCEEDED_EXIT_CODE - + # Print details. for result in results: - if result.test_result.status == Status.FAILURE: - exit_code = PANTS_FAILED_EXIT_CODE + if test_subsystem.options.show_output == ShowOutput.NONE or ( + test_subsystem.options.show_output == ShowOutput.FAILED + and result.test_result.status == Status.SUCCESS + ): + continue has_output = result.test_result.stdout or result.test_result.stderr if has_output: status = ( @@ -328,12 +344,11 @@ async def run_tests( console.print_stderr("") # Print summary - if len(results) > 1: - console.print_stderr("") - for result in results: - console.print_stderr( - f"{result.address.reference():80}.....{result.test_result.status.value:>10}" - ) + console.print_stderr("") + for result in results: + console.print_stderr( + f"{result.address.reference():80}.....{result.test_result.status.value:>10}" + ) merged_xml_results = await Get( Digest, @@ -374,6 +389,12 @@ async def run_tests( if coverage_report_files and test_subsystem.open_coverage: desktop.ui_open(console, interactive_runner, coverage_report_files) + exit_code = ( + PANTS_FAILED_EXIT_CODE + if any(res.test_result.status == Status.FAILURE for res in results) + else PANTS_SUCCEEDED_EXIT_CODE + ) + return Test(exit_code) diff --git a/src/python/pants/core/goals/test_test.py b/src/python/pants/core/goals/test_test.py index 9187c44a00a..c00dd7dc2b9 100644 --- a/src/python/pants/core/goals/test_test.py +++ b/src/python/pants/core/goals/test_test.py @@ -13,6 +13,7 @@ CoverageData, CoverageDataCollection, CoverageReports, + ShowOutput, Status, Test, TestDebugRequest, @@ -137,12 +138,13 @@ def run_test_rule( targets: List[TargetWithOrigin], debug: bool = False, use_coverage: bool = False, + show_output: ShowOutput = ShowOutput.ALL, include_sources: bool = True, valid_targets: bool = True, ) -> Tuple[int, str]: console = MockConsole(use_colors=False) test_subsystem = create_goal_subsystem( - TestSubsystem, debug=debug, use_coverage=use_coverage + TestSubsystem, debug=debug, use_coverage=use_coverage, show_output=show_output, ) interactive_runner = InteractiveRunner(self.scheduler) workspace = Workspace(self.scheduler) @@ -244,11 +246,12 @@ def test_single_target(self) -> None: field_set=SuccessfulFieldSet, targets=[self.make_target_with_origin(address)] ) assert exit_code == 0 - # NB: We don't render a summary when only running one target. assert stderr == dedent( f"""\ ✓ {address} {SuccessfulFieldSet.stdout(address)} + + {address} ..... SUCCESS """ ) @@ -277,6 +280,50 @@ def test_multiple_targets(self) -> None: """ ) + def test_show_output_failed(self) -> None: + good_address = Address.parse(":good") + bad_address = Address.parse(":bad") + + exit_code, stderr = self.run_test_rule( + field_set=ConditionallySucceedsFieldSet, + targets=[ + self.make_target_with_origin(good_address), + self.make_target_with_origin(bad_address), + ], + show_output=ShowOutput.FAILED, + ) + assert exit_code == 1 + assert stderr == dedent( + f"""\ + 𐄂 {bad_address} + {ConditionallySucceedsFieldSet.stderr(bad_address)} + + {good_address} ..... SUCCESS + {bad_address} ..... FAILURE + """ + ) + + def test_show_output_none(self) -> None: + good_address = Address.parse(":good") + bad_address = Address.parse(":bad") + + exit_code, stderr = self.run_test_rule( + field_set=ConditionallySucceedsFieldSet, + targets=[ + self.make_target_with_origin(good_address), + self.make_target_with_origin(bad_address), + ], + show_output=ShowOutput.NONE, + ) + assert exit_code == 1 + assert stderr == dedent( + f"""\ + + {good_address} ..... SUCCESS + {bad_address} ..... FAILURE + """ + ) + def test_debug_target(self) -> None: exit_code, _ = self.run_test_rule( field_set=SuccessfulFieldSet, targets=[self.make_target_with_origin()], debug=True, From db6f4fd85d93cadfec63141e21bc2b90121bb625 Mon Sep 17 00:00:00 2001 From: Benjy Weinberger Date: Mon, 27 Jul 2020 22:34:27 -0700 Subject: [PATCH 2/2] Address code review comments. Also add color to the summary. [ci skip-rust] [ci skip-build-wheels] --- src/python/pants/core/goals/test.py | 20 ++++++++++++++++---- src/python/pants/core/goals/test_test.py | 12 ++++++------ src/python/pants/engine/console.py | 4 ++++ src/python/pants/testutil/engine/util.py | 4 ++-- 4 files changed, 28 insertions(+), 12 deletions(-) diff --git a/src/python/pants/core/goals/test.py b/src/python/pants/core/goals/test.py index 3c03a8b0810..c9d4b0936ef 100644 --- a/src/python/pants/core/goals/test.py +++ b/src/python/pants/core/goals/test.py @@ -237,7 +237,7 @@ def register_options(cls, register) -> None: help="Force the tests to run, even if they could be satisfied from cache.", ) register( - "--show-output", + "--output", type=ShowOutput, default=ShowOutput.FAILED, help="Show stdout/stderr for these tests.", @@ -266,6 +266,10 @@ def debug(self) -> bool: def force(self) -> bool: return cast(bool, self.options.force) + @property + def output(self) -> ShowOutput: + return cast(ShowOutput, self.options.output) + @property def use_coverage(self) -> bool: return cast(bool, self.options.use_coverage) @@ -323,8 +327,8 @@ async def run_tests( # Print details. for result in results: - if test_subsystem.options.show_output == ShowOutput.NONE or ( - test_subsystem.options.show_output == ShowOutput.FAILED + if test_subsystem.options.output == ShowOutput.NONE or ( + test_subsystem.options.output == ShowOutput.FAILED and result.test_result.status == Status.SUCCESS ): continue @@ -346,8 +350,16 @@ async def run_tests( # Print summary console.print_stderr("") for result in results: + color = console.green if result.test_result.status == Status.SUCCESS else console.red + # The right-align logic sees the color control codes as characters, so we have + # to account for that. In f-strings the alignment field widths must be literals, + # so we have to indirect via a call to .format(). + right_align = 19 if console.use_colors else 10 + format_str = f"{{addr:80}}.....{{result:>{right_align}}}" console.print_stderr( - f"{result.address.reference():80}.....{result.test_result.status.value:>10}" + format_str.format( + addr=result.address.reference(), result=color(result.test_result.status.value) + ) ) merged_xml_results = await Get( diff --git a/src/python/pants/core/goals/test_test.py b/src/python/pants/core/goals/test_test.py index c00dd7dc2b9..a46c8167904 100644 --- a/src/python/pants/core/goals/test_test.py +++ b/src/python/pants/core/goals/test_test.py @@ -138,13 +138,13 @@ def run_test_rule( targets: List[TargetWithOrigin], debug: bool = False, use_coverage: bool = False, - show_output: ShowOutput = ShowOutput.ALL, + output: ShowOutput = ShowOutput.ALL, include_sources: bool = True, valid_targets: bool = True, ) -> Tuple[int, str]: console = MockConsole(use_colors=False) test_subsystem = create_goal_subsystem( - TestSubsystem, debug=debug, use_coverage=use_coverage, show_output=show_output, + TestSubsystem, debug=debug, use_coverage=use_coverage, output=output, ) interactive_runner = InteractiveRunner(self.scheduler) workspace = Workspace(self.scheduler) @@ -280,7 +280,7 @@ def test_multiple_targets(self) -> None: """ ) - def test_show_output_failed(self) -> None: + def test_output_failed(self) -> None: good_address = Address.parse(":good") bad_address = Address.parse(":bad") @@ -290,7 +290,7 @@ def test_show_output_failed(self) -> None: self.make_target_with_origin(good_address), self.make_target_with_origin(bad_address), ], - show_output=ShowOutput.FAILED, + output=ShowOutput.FAILED, ) assert exit_code == 1 assert stderr == dedent( @@ -303,7 +303,7 @@ def test_show_output_failed(self) -> None: """ ) - def test_show_output_none(self) -> None: + def test_output_none(self) -> None: good_address = Address.parse(":good") bad_address = Address.parse(":bad") @@ -313,7 +313,7 @@ def test_show_output_none(self) -> None: self.make_target_with_origin(good_address), self.make_target_with_origin(bad_address), ], - show_output=ShowOutput.NONE, + output=ShowOutput.NONE, ) assert exit_code == 1 assert stderr == dedent( diff --git a/src/python/pants/engine/console.py b/src/python/pants/engine/console.py index 6a08a4ab58f..03e92e32ec4 100644 --- a/src/python/pants/engine/console.py +++ b/src/python/pants/engine/console.py @@ -95,6 +95,10 @@ def flush(self) -> None: self.stdout.flush() self.stderr.flush() + @property + def use_colors(self): + return self._use_colors + def _safe_color(self, text: str, color: Callable[[str], str]) -> str: """We should only output color when the global flag --colors is enabled.""" return color(text) if self._use_colors else text diff --git a/src/python/pants/testutil/engine/util.py b/src/python/pants/testutil/engine/util.py index d88fdd652a9..7fba66fea56 100644 --- a/src/python/pants/testutil/engine/util.py +++ b/src/python/pants/testutil/engine/util.py @@ -235,7 +235,7 @@ class MockConsole: def __init__(self, use_colors=True): self.stdout = StringIO() self.stderr = StringIO() - self._use_colors = use_colors + self.use_colors = use_colors def write_stdout(self, payload): self.stdout.write(payload) @@ -250,7 +250,7 @@ def print_stderr(self, payload): print(payload, file=self.stderr) def _safe_color(self, text: str, color: Callable[[str], str]) -> str: - return color(text) if self._use_colors else text + return color(text) if self.use_colors else text def blue(self, text: str) -> str: return self._safe_color(text, blue)