Don't give a subsequent formatter an empty snapshot after skipping #15483
Conversation
# 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]
| @@ -15,7 +15,7 @@ | |||
| from pants.engine.fs import EMPTY_DIGEST, CreateDigest, Digest, FileContent | |||
There was a problem hiding this comment.
This file could use a good amount of TLC. It seems to under-test fmt pretty badly and has more than a few quirks.
It's out of scope for this PR though. I cleaned it up enough to test the fix (fails before, passes after)
thejcannon
added
category:bugfix
| @@ -286,7 +286,8 @@ async def fmt_language(language_fmt_request: _LanguageFmtRequest) -> _LanguageFm | |||
| continue | |||
| result = await Get(FmtResult, FmtRequest, request) | |||
| results.append(result) | |||
| prior_formatter_result = result.output | |||
| if not result.skipped: | |||
| prior_formatter_result = result.output | |||
There was a problem hiding this comment.
Question for @Eric-Arellano @stuhood
Is the intention for skipped formatters to return an empty digest or just forward through the original input digest (untouched)?
As in, is the core intention for formatters to be pipelined (in some deterministic order), or is this a workaround/side-effect because skipped formatters happen to have empty digests?
There was a problem hiding this comment.
It's a good point. Right now "skipped" is a magical combo of values. What would it look like if it was distinct?
There was a problem hiding this comment.
Yeah I don't think it matters much how Skip is factored, as long we don't waste time trying to run it etc
|
@thejcannon Per our Slack chat - how is the console output of a skipped test handled? Citing my original ticket:
|
|
wack |
And then it passes. Flaky? |
| @@ -286,7 +286,8 @@ async def fmt_language(language_fmt_request: _LanguageFmtRequest) -> _LanguageFm | |||
| continue | |||
| result = await Get(FmtResult, FmtRequest, request) | |||
| results.append(result) | |||
| prior_formatter_result = result.output | |||
| if not result.skipped: | |||
| prior_formatter_result = result.output | |||
There was a problem hiding this comment.
Yeah I don't think it matters much how Skip is factored, as long we don't waste time trying to run it etc
…antsbuild#15483) Fixes pantsbuild#15406 by not piping a skipped formatter result into the next formatter. [ci skip-rust] [ci skip-build-wheels]
…herry-pick of #15483) (#15602) Don't give a subsequent formatter an empty snapshot after skipping (#15483) Fixes #15406 by not piping a skipped formatter result into the next formatter. [ci skip-rust] [ci skip-build-wheels] Co-authored-by: Joshua Cannon <[email protected]>
Fixes #15406 by not piping a skipped formatter result into the next formatter.
[ci skip-rust]
[ci skip-build-wheels]