RFC: providing coverage when recipes move out of build

[Paweł Hajdan, Jr.]

See an example at the end of this email how e.g. removing the chromium recipe from build repo would affect recipe code coverage there. Note that most builders are now using the recipe in build - although it only invokes one method in chromium_tests to avoid duplicating the code.

I'm wondering how can we solve the coverage issue to allow the recipes to move (which means deleting them from build). I can produce a doc if discussion gets more complicated.

Some options I see:

1) Add "pragma: no cover". This is "obvious" and mechanical (easy to apply). However, I'm seriously concerned this would make ineffective any benefits we get from enforcing code coverage in recipes. I listed this for completeness.

2) Keep the chromium recipe in build repo until we can move more code to chromium/src. This also seems easy, but we'd hit similar issues trying to remove e.g. findit or bisection recipes. I don't think this solution would be generally applicable.

3) Add a way to specify downstream repos in recipes.cfg. Their tests would be executed and count towards code coverage. This may be a non-trivial change to the recipe engine. With support for fetching from gitiles, this is realistic even for otherwise huge repos like chromium/src. Now there's a downside: changes in this downstream repo can now result in coverage problems upstream.

4) Add more code to example recipes for corresponding modules. A possible downside is that some code may end up only being executed by the examples. If downstream expectations change, that also gives us real coverage in a way.

We'll need to solve this somehow to proceed with recipes-in-repos. If you see more options, please share.

Options 1 and 2 are here for completeness. I don't think they'd be applicable here, even short term. I'm considering options 3 and 4, leaning towards the last one (just adding more example code).

To give you a specific example, this is the result of removing chromium recipe from build repo and running recipe simulation tests. I was planning to move chromium_tests to chromium repo, but not the other modules yet.

Coverage Report                                                       Stmts   Miss  Cover   Missing

---------------------------------------------------------------------------------------------------

scripts/slave/recipe_modules/archive/api                                147     22    85%   120, 122, 126, 128-150, 270-272

scripts/slave/recipe_modules/chromium/api                               369     21    94%   76, 197, 213, 215, 217, 219, 434-435, 733-758, 770

scripts/slave/recipe_modules/chromium/config                            343     36    90%   251-252, 256, 264-265, 352-353, 420, 523-524, 529, 542, 550, 554, 558, 562, 566, 570, 580, 584, 589, 594, 598, 605, 610, 616, 628, 690-691, 700, 771, 784-787, 791

scripts/slave/recipe_modules/chromium_android/api                       581     10    98%   588, 696, 1127-1135

scripts/slave/recipe_modules/chromium_android/chromium_config            73      1    99%   198

scripts/slave/recipe_modules/chromium_android/config                     68      2    97%   214, 222

scripts/slave/recipe_modules/chromium_tests/api                         400     64    84%   165, 300-301, 305-335, 437-499, 534, 555-556, 566, 585, 740, 757-766, 825-853

scripts/slave/recipe_modules/chromium_tests/bot_config_and_test_db       93      3    97%   67, 73, 187

scripts/slave/recipe_modules/chromium_tests/chromium_perf_fyi            34      2    94%   21, 26

scripts/slave/recipe_modules/chromium_tests/steps                       738    142    81%   23, 81, 91, 99, 192, 354, 406, 435, 455, 466-471, 502-517, 528, 570, 574, 577-582, 585-601, 604, 613-635, 639, 669-688, 693, 857, 862, 870, 872, 960-965, 969, 973, 976, 980, 983-985, 992-1023, 1026-1030, 1035, 1056, 1083-1088, 1130-1135, 1149, 1262-1274, 1285, 1391, 1396, 1400, 1574, 1607, 1669, 1673, 1677, 1680-1682, 1725, 1728-1746

scripts/slave/recipe_modules/goma/api                                    98      3    97%   90, 110-112

scripts/slave/recipe_modules/isolate/api                                 73      6    92%   192, 196-204

scripts/slave/recipe_modules/swarming/api                               351      7    98%   739, 743-744, 788, 803-805

scripts/slave/recipe_modules/test_utils/test_api                         80      9    89%   47, 56-59, 134-135, 153, 158

---------------------------------------------------------------------------------------------------

TOTAL                                                                 16494    328    98% 

Paweł

[Robert Iannucci]

Couldn't these chromium-only bits of code move when the recipe moves (maybe to a new module)? It seems wrong that a downstream repo's usage of an upstream repo's modules should count towards coverage, no matter the mechanism.

If the behavior of the upstream module is well-defined, however, having a sensible example that covers it seems fine (think of how coverage counts inside of infra.git... tests for a module cover just that module, even if the module is used elsewhere in the codebase). I think there's still a way to have multiple example recipes per module, but I'm not sure that anything used that feature. It would definitely be a better way to do it than cramming all example usage into a single non-sensical recipe :).

Allowing a different prefix than 'example' would probably be a good idea too. Maybe a tests/ subdirectory under the recipe module where each file is a recipe that tests an aspect of the module?

R

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPYtVzHaHDz-1ACi8Pi028W%2BM-AfO%3D5Sw6%3DaKXgQ31%2BwSQ%40mail.gmail.com.

[Sergey Berezin]

It seems recipes in their current state mix unit tests with config change detection, basically using the latter in place of the former. Why are recipes so special? Can we just write actual unit tests for them and stop using production configs for coverage? And if we find value in config change detection (do we?), maybe we should move those expectations next to the configs somehow, and decouple them from recipes. WDYT?

Just my very high-level $.02, I don't know how much work all this may involve, or if it even makes sense. But at a very high level, I don't understand why unit tests should block moving code from one repo to another.

Sergey.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CA%2Bq_oBdUyFCOZZBb-3ujXCOrHhMtZuLhNq679Ds4w-Zk2wakcQ%40mail.gmail.com.

[Robert Iannucci]

Unit tests for recipes been discussed a lot before, and I'm sure will be discussed more in the future. The biggest hurdle to a real 'unit test' would be developing realistic mocks.

However, we SHOULD use the existing capabilities of the recipes system to make modules behave like independently-tested modules.

These modules should have their own representative tests in place, such that their behavior is well defined, regardless of which recipes use them. I think this is very similar to what I proposed (adding more per-module tests).

In the longer term, we should consider making it so that recipes exercising a module do not count towards that module's coverage statistics. We should also introduce a 'dead code finder' mode which runs all the recipes (in the entire dependency graph) to try to root out code in modules which is actually no longer used (currently the combined-coverage functions in this capacity somewhat).

So I think the recommendation is the same: add tests (currently via example.py, but adding a tests folder would also be a good idea) to the modules so that the modules have independent test verification. I will take a stab at adding a tests folder after I'm done rooting out all the evils in bot_update.

R

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAPcKkT%3DEkWEYqWXO72ca%3De_QCGuCiUvWNgW_af518GGrgorTSw%40mail.gmail.com.

[Stephen Martinis]

+1 to robbie's email. I think the solution right now is to cover everything with example.py, as annoying as that is :/

[Robert Iannucci]

Added a bug re: stronger testing in recipe modules: https://bugs.chromium.org/p/chromium/issues/detail?id=645293

[Paweł Hajdan, Jr.]

I'll be looking in more detail for the best way to provide coverage in build repo once chromium recipe is deleted from it.

Meanwhile, I'd like to seek your opinion about the following:

1. We could add scripts/slave/recipes/examples directory and put some recipes there.

2. I'm thinking about the best name for recipe subdirectory inside recipe modules. How about "recipes"? Given one can run recipes using recipe:module syntax, in the future we might move all recipes to some modules. As alternatives, how about already mentioned "tests" or possibly "examples"?

3. I like the idea of recipe execution not counting towards module coverage. Would you be OK with me making such change? We'd probably need some kind of switch or flag to opt-in repos when they're ready. Such flag could live in recipes.cfg. With recipes-in-repos such change would definitely help move things around.

I also like the idea of 'dead code finder' across all repos.

Note some parts of recipe code already have unit tests, for example https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/auto_bisect/bisector_test.py?sq=package:chromium&dr=CSs .

Sergey, could you elaborate on "mixing unit tests with config change detection" and "move those expectations next to the configs somehow, and decouple them from recipes"? What configs are you refering to? One thing would be recipes config.py files. It might also be per-waterfall bot configs in chromium_tests recipe module. I'd like to better understand your suggestions.

Paweł

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPYtVzHaHDz-1ACi8Pi028W%2BM-AfO%3D5Sw6%3DaKXgQ31%2BwSQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CA%2Bq_oBdUyFCOZZBb-3ujXCOrHhMtZuLhNq679Ds4w-Zk2wakcQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+unsubscribe@chromium.org.

To post to this group, send email to infr...@chromium.org.

[Robert Iannucci]

On Fri, Sep 9, 2016 at 1:54 PM Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I'll be looking in more detail for the best way to provide coverage in build repo once chromium recipe is deleted from it.

Meanwhile, I'd like to seek your opinion about the following:

1. We could add scripts/slave/recipes/examples directory and put some recipes there.

Maybe scripts/slave/recipes/module_tests/<module_name>/* for now, but this sgtm. "examples" were really meant to be examples for how a human might use the module, but have turned into (often confusing) test-like things. Separating the two ideas would be good.

 

2. I'm thinking about the best name for recipe subdirectory inside recipe modules. How about "recipes"? Given one can run recipes using recipe:module syntax, in the future we might move all recipes to some modules. As alternatives, how about already mentioned "tests" or possibly "examples"?

Yes, I was thinking along these lines too, but didn't want to get ahead of myself :D. I think there may still be some value of explicitly calling out tests v. 'usable' recipes though. Maybe both '/tests' and '/recipes', with 'tests/example.py' being the conventional example for a module, but otherwise having no special powers.

I think 'tests' may later then be used to indicate which recipes should be run on a real bot for the purpose of integration testing (e.g. for bot_update). I'd really like to set up easy _real_ waterfall testing for modules, particularly infra-maintained ones. I wouldn't want to require that all project code have this level of testing, but I think we should be able to facilitate it if various projects do want to take advantage of it.

 

3. I like the idea of recipe execution not counting towards module coverage. Would you be OK with me making such change? We'd probably need some kind of switch or flag to opt-in repos when they're ready. Such flag could live in recipes.cfg. With recipes-in-repos such change would definitely help move things around.

That would be fantastic! I think it'll need to be done carefully though; I suspect that all of the modules we have today have at least some only-covered-by-recipes lines, so we'll definitely need to have it be opt-in and then burn down all the modules until we can remove the opt-in flag and make it permanently enabled. I'm kicking myself for not making this the mode of operation earlier, but I was young and naïve :(

 

I also like the idea of 'dead code finder' across all repos.

Yeah the current coverage policy (e.g. everything covers everything) is convoluted with this functionality, but running it as an explicit, separate tool (maybe continuously on a waterfall somewhere!) would be a much better story (lets tests be tests, dead code finder be a dead code finder).

Eventually I'd like to get a feedback loop going of real recipe runs -> GenTests so we can also identify when the simulated runs of recipes diverge from reality. I suspect that un-updated simulation tests also mask dead code right now, and exhibit inaccuracy (which reduces the usefulness of the predictive power of the simulations :( ).

 

Note some parts of recipe code already have unit tests, for example https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/auto_bisect/bisector_test.py?sq=package:chromium&dr=CSs .

Yep, though there's no real support from the recipe engine for finding and running them. Having the recipe engine be involved would also allow these unit tests to count towards the module's coverage statistics I think, which would be good. That way truly unit-testable segments (e.g. transform inputs into outputs, calculate something) could be tested with unit tests, and the systems-integration stuff (running steps) could be tested with expectations.

It might also allow us to include resource scripts (e.g. bot_update.py) in the coverage statistics too.

 

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPYtVzHaHDz-1ACi8Pi028W%2BM-AfO%3D5Sw6%3DaKXgQ31%2BwSQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CA%2Bq_oBdUyFCOZZBb-3ujXCOrHhMtZuLhNq679Ds4w-Zk2wakcQ%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAPcKkT%3DEkWEYqWXO72ca%3De_QCGuCiUvWNgW_af518GGrgorTSw%40mail.gmail.com.

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.

To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.

To post to this group, send email to infr...@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPbdy7DR9w7S%2B84Bwz2ABRFaZv5BHXVbQqf8GiONthhp9w%40mail.gmail.com.