Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stream output of test, rather than dumping at the end #10634

Merged
merged 4 commits into from Aug 18, 2020
Merged

Stream output of test, rather than dumping at the end #10634

merged 4 commits into from Aug 18, 2020

Conversation

Eric-Arellano
Copy link
Contributor

@Eric-Arellano Eric-Arellano commented Aug 17, 2020 •
edited

Before:

▶ ./pants test src/python/pants/util/{dir,str}util_test.py --test-output=all
11:20:56.32 [INFO] Completed: Run Pytest for src/python/pants/util/dirutil_test.py:tests
11:20:56.99 [INFO] Completed: Run Pytest for src/python/pants/util/strutil_test.py:tests
11:20:56.99 [ERROR] Completed: Run Pytest - tests failed (exit code 1): src/python/pants/util/strutil_test.py:tests
✓ src/python/pants/util/dirutil_test.py:tests
============================= test session starts ==============================
collected 31 items

src/python/pants/util/dirutil_test.py ...............................    [100%]

============================== 31 passed in 0.12s ==============================


𐄂 src/python/pants/util/strutil_test.py:tests
============================= test session starts ==============================
collected 5 items

src/python/pants/util/strutil_test.py ..F..                              [100%]

=================================== FAILURES ===================================
__________________________ StrutilTest.test_pluralize __________________________

self = <pants.util.strutil_test.StrutilTest testMethod=test_pluralize>

    def test_pluralize(self) -> None:
        self.assertEqual("1 bat", pluralize(1, "bat"))
        self.assertEqual("1 boss", pluralize(1, "boss"))
        self.assertEqual("2 bats", pluralize(2, "bat"))
        self.assertEqual("2 bosses", pluralize(2, "boss"))
>       self.assertEqual("0 batz", pluralize(0, "bat"))
E       AssertionError: '0 batz' != '0 bats'
E       - 0 batz
E       ?      ^
E       + 0 bats
E       ?      ^

src/python/pants/util/strutil_test.py:23: AssertionError
=========================== short test summary info ============================
FAILED src/python/pants/util/strutil_test.py::StrutilTest::test_pluralize - A...
========================= 1 failed, 4 passed in 0.12s ==========================


src/python/pants/util/dirutil_test.py:tests                                     .....   SUCCESS
src/python/pants/util/strutil_test.py:tests                                     .....   FAILURE

After:

▶ ./pants test src/python/pants/util/{dir,str}util_test.py --test-output=all
11:20:08.22 [INFO] Completed: Run tests - src/python/pants/util/dirutil_test.py:tests succeeded.
============================= test session starts ==============================
collected 31 items

src/python/pants/util/dirutil_test.py ...............................    [100%]

============================== 31 passed in 0.12s ==============================

11:20:08.27 [WARN] Completed: Run tests - src/python/pants/util/strutil_test.py:tests failed (exit code 1).
============================= test session starts ==============================
collected 5 items

src/python/pants/util/strutil_test.py ..F..                              [100%]

=================================== FAILURES ===================================
__________________________ StrutilTest.test_pluralize __________________________

self = <pants.util.strutil_test.StrutilTest testMethod=test_pluralize>

    def test_pluralize(self) -> None:
        self.assertEqual("1 bat", pluralize(1, "bat"))
        self.assertEqual("1 boss", pluralize(1, "boss"))
        self.assertEqual("2 bats", pluralize(2, "bat"))
        self.assertEqual("2 bosses", pluralize(2, "boss"))
>       self.assertEqual("0 batz", pluralize(0, "bat"))
E       AssertionError: '0 batz' != '0 bats'
E       - 0 batz
E       ?      ^
E       + 0 bats
E       ?      ^

src/python/pants/util/strutil_test.py:23: AssertionError
=========================== short test summary info ============================
FAILED src/python/pants/util/strutil_test.py::StrutilTest::test_pluralize - A...
========================= 1 failed, 4 passed in 0.17s ==========================


✓ src/python/pants/util/dirutil_test.py:tests succeeded.
𐄂 src/python/pants/util/strutil_test.py:tests failed.

We still respect the option --test-output={all,success,none}. To support this, we add an EnrichedTestResult type and automatically upcast a TestResult to this type.

We also now group all failures together at the end of the summary.

[ci skip-rust]

@Eric-Arellano
Stream output of test, rather than dumping at the end
5da01a1 
[ci skip-rust]
[ci skip-build-wheels]
@Eric-Arellano
Improve test summary
c225382 
* Group failures together at the end
* Align summary with `fmt` and `lint` style

# 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]
Eric-Arellano
Eric-Arellano commented Aug 17, 2020
View reviewed changes
src/python/pants/core/goals/test.py

@dataclass(frozen=True)
class EnrichedTestResult(EngineAware):
exit_code: Optional[int]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stu pointed out in #10632 (comment) that using the null pattern with Optional[int] to represent "skipped" is not great modeling. Alternatively, we could have an OptionalTestResult top-level type to keep the model more correct.

I think it would indeed be more correct, but my concern is that it makes test.py and each test implementation a lot clunkier.

@Eric-Arellano
Fix legit rule graph issue with TestResult not being reachable
fe6189f 
# 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]
@Eric-Arellano
Fix log_output_integration_test.py
ca66dc2 
We now set the Pytest process to Debug level, so the workunit doesn't get outputted like this test expects.

I tried simply having the test use `-ldebug`, but it didn't work properly. For some reason, the log message doesn't show up in the test's stderr, even though it works properly in production.

# 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]
@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling ca66dc2 on Eric-Arellano:stream-test into 1e1190d on pantsbuild:master.

@Eric-Arellano Eric-Arellano merged commit 2ca63be into pantsbuild:master Aug 18, 2020
@Eric-Arellano Eric-Arellano deleted the stream-test branch August 18, 2020 02:49
@Eric-Arellano Eric-Arellano mentioned this pull request Aug 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjyw benjyw benjyw approved these changes

@stuhood stuhood Awaiting requested review from stuhood

@gshuflin gshuflin Awaiting requested review from gshuflin

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Eric-Arellano @coveralls @benjyw