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.

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

Add duration and cache source to test output. #13889

Merged
merged 4 commits into from
Dec 27, 2021

Conversation

riisi
Copy link
Contributor

@riisi riisi commented Dec 15, 2021

Fixes #13588.

[ci skip-rust]
[ci skip-build-wheels]

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

benjyw commented Dec 20, 2021

@riisi Thanks for this contribution! I think a lot of users will love this, and it helps demonstrate one of the major benefits of Pants.

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

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

Thanks a lot @riisi! This looks great.

Comment on lines 161 to 162
# TODO: What should be here?
result_metadata=ProcessResultMetadata(None, "", 0),
Copy link
Member

Choose a reason for hiding this comment

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

Good question. I think that:

  1. this case of failed compilation (where the test process didn't even start)
  2. the case of a skipped test (TestResult.skip)

...could maybe both be joined into an explicit yellow "was skipped" state for the test. For now, it seems simple enough to make the result_metadata optional (result_metadata: ProcessResultMetadata | None), and to then render the skipped sigil for tests with no metadata:

if results.skipped:
sigil = console.sigil_skipped()
status = "skipped"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure how to replicate skipped tests for Python. I think in the case of using pytest.mark.skip to skip a module, then pytest exits with non-0 code - I think this has been discussed in Slack before.
Haven't tested failed compilation yet.

src/python/pants/core/goals/test.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/test.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/test_test.py Outdated Show resolved Hide resolved
src/python/pants/core/goals/test.py Outdated Show resolved Hide resolved
@riisi
Copy link
Contributor Author

riisi commented Dec 24, 2021

Thanks for this @stuhood. I'll start taking a look now - internet access maybe a bit restricted over holiday period though.

riisi and others added 2 commits December 24, 2021 10:26
Co-authored-by: Stu Hood <stuhood@gmail.com>
# 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]
@riisi
Copy link
Contributor Author

riisi commented Dec 24, 2021

Some mypy issues to fix - can look into this further. Pushed what I have for now.

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

stuhood commented Dec 27, 2021

Some mypy issues to fix - can look into this further. Pushed what I have for now.

I've fixed the typing issues, and added output support for skipped tests.

Thanks a lot for the contribution!

@stuhood stuhood enabled auto-merge (squash) December 27, 2021 19:47
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Sweet! Thanks!

@stuhood stuhood added this to the 2.9.x milestone Dec 27, 2021
@stuhood stuhood merged commit fa74bf4 into pantsbuild:main Dec 27, 2021
stuhood pushed a commit to stuhood/pants that referenced this pull request Dec 27, 2021
stuhood added a commit that referenced this pull request Dec 27, 2021
@riisi
Copy link
Contributor Author

riisi commented Dec 28, 2021

Thanks @stuhood ! 👏

asherf added a commit to asherf/pants that referenced this pull request Dec 28, 2021
Eric-Arellano pushed a commit that referenced this pull request Dec 29, 2021
…call to _format_test_summary (#14019)

instead of on every call to _format_test_summary

follow up for #13889
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add output to signal if a test result was cached
4 participants