https://pantsbuild.org/ logo
#development
Title
# development
w

witty-crayon-22786

02/06/2020, 6:13 PM
@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

hundreds-father-404

02/06/2020, 6:16 PM
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

witty-crayon-22786

02/06/2020, 6:24 PM
basically, i think that whatever hacks are necessary to allow the flag and goal to live in the right place is critical
d

dry-analyst-73584

02/06/2020, 6:24 PM
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

witty-crayon-22786

02/06/2020, 6:24 PM
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

dry-analyst-73584

02/06/2020, 6:25 PM
This will break pants v2 for anyone without the pants.backend.python backend installed.
Which was why we stopped.
w

witty-crayon-22786

02/06/2020, 6:25 PM
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

dry-analyst-73584

02/06/2020, 6:27 PM
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

witty-crayon-22786

02/06/2020, 6:28 PM
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

hundreds-father-404

02/06/2020, 6:30 PM
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
h

hundreds-father-404

02/06/2020, 6:31 PM
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

witty-crayon-22786

02/06/2020, 6:32 PM
we could, but that is less desirable than having rule-internal/privatish hacks
h

hundreds-father-404

02/06/2020, 6:32 PM
(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

witty-crayon-22786

02/06/2020, 6:34 PM
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

dry-analyst-73584

02/06/2020, 6:39 PM
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

witty-crayon-22786

02/06/2020, 6:42 PM
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

dry-analyst-73584

02/06/2020, 6:44 PM
👍
Tonight I have an event to go speak at so this works well too 🙂
🔥 1
a

aloof-angle-91616

02/06/2020, 6:49 PM
i'll probably be around this evening too if i can make enough progress on something else today