https://pantsbuild.org/ logo
#general
Title
# general
p

plain-leather-21357

12/06/2018, 8:28 PM
I have a question about filter and minimize minimum set coverage for
go_library
targets. Basically, when I run
filter-minimize
on my repo dir containing my go corpus, the minimized target list is ~33% of the actual list (which could be ok, maybe), and excludes targets that have
go_library
BUILD files and tests. The result is that there are well-defined
go_library
targets that contain tests, which aren’t covered when discovering targets via minimize
w

witty-crayon-22786

12/06/2018, 8:30 PM
minimize computes the covering set of targets, so it's expected that the set is smaller
p

plain-leather-21357

12/06/2018, 8:30 PM
right, but, it seems it does not meet the covering set criteria
w

witty-crayon-22786

12/06/2018, 8:30 PM
but presumably
./pants test $go_library
is not testing things transitively...
@plain-leather-21357: do the targets that are minimized away have dependees within the set?
if the answer is yes, then it's computing the covering set correctly.
@enough-analyst-54434: as a strawman, any thoughts on injecting
go_test
targets next to all
go_library
targets?
the issue with
./pants test $go_library
testing transitively for go would be that it would cause redundancy when you do the standard pants ci dance: https://www.pantsbuild.org/orgs.html#continuous-integration
p

plain-leather-21357

12/06/2018, 8:33 PM
I think so, e.g. one target I found that is not being tested, is listed as a dependency in a target that is tested/output by minimize
however, the tests don’t get run when the dependee is being tested
w

witty-crayon-22786

12/06/2018, 8:34 PM
@plain-leather-21357: right: if they did, you'd have a lot of redundancy in CI
adding "test transitively" as an option for the go test runner would be an option to stop the bleeding, but we would need to change the graph shape to avoid redundant test running, i think
p

plain-leather-21357

12/06/2018, 8:37 PM
makes sense, although I don’t have enough context about the graph shape part
is test transitive an existing option?
w

witty-crayon-22786

12/06/2018, 8:44 PM
@plain-leather-21357: no, would have to add it.
you see the issue though, right? you'd run your core-library tests hundreds or thousands of times (unless you have remote test caching)
p

plain-leather-21357

12/06/2018, 8:45 PM
right
w

witty-crayon-22786

12/06/2018, 8:46 PM
the graph shape thing would be to add something like
go_test
(likely automatically...?), and then those would be your minimization roots, and they would have no dependents
p

plain-leather-21357

12/06/2018, 8:47 PM
ahh, that makes sense
w

witty-crayon-22786

12/07/2018, 8:26 PM
@enough-analyst-54434: guessing you're climbing, but if you have any thoughts later this evening, let us know
p

plain-leather-21357

12/08/2018, 1:00 AM
note that we should also include
go_binary
in this, as those targets can also have tests
e

enough-analyst-54434

12/08/2018, 6:47 PM
the issue with
./pants test $go_library
testing transitively for go would be that it would cause redundancy when you do the standard pants ci dance: https://www.pantsbuild.org/orgs.html#continuous-integration
That's if we don't implement self.invalidated checking under the v1 engine for single shard testing and if we don't implement test result caching under the v1 engine for sharded testing - which our go support in OSS doesn't support anyhow. So I think adding a transitive flag to
test.go
and implementing a self.invalidated guard is a viable option. Looking at history, the non-transitive nature of the task was an original decision in 2015 by Cody perhaps ripe for revisit.
w

witty-crayon-22786

12/10/2018, 10:15 PM
yea, i suppose so. i guess it's not a huge amount of work to add test caching there, although turning on test caching safely still requires
--chroot
and etc.
not sure whether go tests are already chrooted.
p

plain-leather-21357

12/14/2018, 6:22 PM
How big an effort does this seem like it is? Any idea on when it’d be looked at?
w

witty-crayon-22786

12/14/2018, 6:27 PM
@plain-leather-21357: slightly unclear re:
chrooting
... our internal tickets on the topic indicate that something like 1% of tests fail under chrooting
the actual implementation of that solution is not very large... the question would mostly be about whether the tests already work under chrooting
e

enough-analyst-54434

12/14/2018, 6:28 PM
I muddied things above. Under v1 caching is not a thing here, just invalidation. We could implement this with invalidation only.
w

witty-crayon-22786

12/14/2018, 6:29 PM
caching is relevant in a distributed environment though
e

enough-analyst-54434

12/14/2018, 6:29 PM
But v1 and go test does not support sharding - right - so it doesn;'t matter until v2
IE in your distributed ci today, all go tests must run on 1 machine unless you shard at a higher scripting layer than pants.
w

witty-crayon-22786

12/14/2018, 6:35 PM
and we do, because we do https://www.pantsbuild.org/orgs.html#continuous-integration with the for-loop replaced with parallel/distributed test running
e

enough-analyst-54434

12/14/2018, 6:36 PM
k
w

witty-crayon-22786

12/14/2018, 6:41 PM
ok, i think we have the information to create a ticket for this one. thanks everyone!
e

enough-analyst-54434

12/14/2018, 6:42 PM
Injecting go_test targets for the purpose of getting minimize to work definitely seems hacky. Adjusting the ci script to handle go seperately, turning minimial cover into all interior targets too, then adding that to the list - seems preferrable from a pants end but probably not your end.
Basically, using a custom version of minimize seems nicest to me here. That falls down the minute you get go shop #2 though since it would be a private minimize v1 task plugin.
w

witty-crayon-22786

12/14/2018, 6:45 PM
sorry, didn't see the last sentence in here before creating
we'll get someone from twitter on that at some point
3 Views