https://pantsbuild.org/ logo
Join Slack
Powered by
<@U098KD4HJ>: re: using a `CoverageResult` union f...
# development
w
@dry-analyst-73584: re: using a 
CoverageResult
union for https://github.com/pantsbuild/pants/pull/8978 : what trouble did you run into?
related: even if there are some python-specific bits in the core 
test
rules, that would still be preferable to a separate goal, i think
h
I’ll backtrack our conversation with Greg from last week. @hundreds-breakfast-49010 do you happen to remember why we realized it wouldn’t work as expected?
w
basically, i think that whatever hacks are necessary to allow the flag and goal to live in the right place is critical
d
We set up a 
CoverageResult
union, but could not 
await Get[CoverageResult]
in the generic test runner because the only way to get a 
CoverageResult
was in the pants.backend.python backend, which is not always installed.
w
i expect that the union thing can be figured out, but if in the meantime we need python bits in core, that's ok
d
This will break pants v2 for anyone without the pants.backend.python backend installed.
Which was why we stopped.
w
UnionMembership will allow you to check whether you have members: if you don't, you skip coverage (edit: thinking about this more, i can imagine what you ran into)
because we have lots of time to refactor the rules, but if a really different CLI interface is exposed now, there is a definite risk that we'll lose the motivation to fix it
d
The build graph doesn't know that though - I had the 
await Get
inside a check for whether the 
--run-coverage
option was passed, but the build graph sees that it requests a 
CoverageResult
and fails.
👍 1
w
yea, edited the above. it's fine to land with workarounds that allow the options/goal to live in the right place, and a TODO pointing to a ticket.
@dry-analyst-73584: does that make sense?
basically, if you need to put "PythonCoverageResult" in the core, then ok-fine-lets-do-it-for-now with a TODO
(as an Optional field on the TestResult, for example)
we can discuss the union thing independently, because i think i know how it would work.
but it doesn't need to block if the UX is as-designed.
h
PythonCoverageResult
Originally, we did do this. But the issue is that we must provide a no-op way to 
await Get[PythonCoverageResult]
when 
pants.backend.python
is not registered. If we provide a no-op, then the rule graph would always use that rule because it’s the simplest path, rather than having to call the Python rules
w
h
Can we rename the goal to be something very hacky and perhaps even frustrating to type so that a) users know this is a WIP and b) we don’t settle with the UX? Something like 
v2-coverage
or 
pytest-cov2
w
we could, but that is less desirable than having rule-internal/privatish hacks
h
(as an Optional field on the TestResult, for example)
We already have that for an individual 
TestResult
. But we need some way to collect and merge all the `TestResult.coverage_digest`s into one single 
CoverageResport
. So, we would have to create a new type. This is where we ran into the issues with needing a no-op implementation
w
ok. that's the using a union as a field case, i think.
i'm not fully understanding what failed in the rule graph, i think. if a union is empty, the 
yield Get[X]($union)
"disappears" from the graph... there is no validation, effectively
cf: the test runner itself
so i think there are probably minor tweaks there
@dry-analyst-73584: i won't be able to work on it during the day, but if you post a failing version of this as one commit atop your working PR, i can poke at it a bit after hours?
d
Sure 🙂 I would be happy to pair on it as well if that's useful, I would love to know how to solve this!
❤️ 1
w
i won't be able to pair today, unfortunately... i really need to make progress on some other work i've been putting off
but i'll sit in a recorded video chat and talk to myself this evening if anyone is around. either way, can post it
d
👍
Tonight I have an event to go speak at so this works well too 🙂
🔥 1
a
i'll probably be around this evening too if i can make enough progress on something else today
Open in Slack
PreviousNext
Powered by