RFC: Adopt Codecov at Artsy, starting with Gravity

lidimayra commented 3 years ago

Proposal:

Adopt Codecov at Artsy, starting with Gravity

Configure codecov to comment in the PRs with coverage results and link to codecov results page
If a PR is opened that decreases the test coverage, should we block it?
- No, enforcing a hard block would probably lead to more frustration without incentivizing us to improve our tests
Why start with Gravity?
- Code coverage tools seem to be most useful for backends
- We could consider adopting for our frontends, but it seems like it might be harder to adopt consistently and with good results

Reasoning:

Helps us to see when our code is poorly structured: if you feel that covering all of a class is difficult, it might mean that the class needs to be refactored!
Enables us to refactor with more confidence by knowing which parts of code are not covered. Even though Codecov won't force us to have high-quality tests (this is something to be enforced by ourselves when writing and reviewing code), it will give us visibility on what's tested and what's not.
Allows new engineers to better understand the code by reading tests as documentation
Makes major dependency updates easier to be tested
Helps us see where there are gaps in our test coverage - even if we choose not to add tests where we're missing them, we can make sure to manually test those portions of a code change

Exceptions:

None

Additional Context:

https://about.codecov.io/

How is this RFC resolved?

Deciding whether we want to adopt codecov or not and in case we do start this setting with Gravity as an experiment.

kajatiger commented 3 years ago

I definitely like that it will spare the reviewer the hint that there is a test missing for a new implementation. I like when tools are taking over these kind of simple jobs. In the past I have had different experiences... Sometimes it was useful, and sometimes the tool kept being ignored and in the end was just hindering work. I think it also depends on the configuration and usage. But I am definitely up for a try.

anandaroop commented 2 years ago

I'm intrigued. We've done this very sporadically for some projects (example — coveralls instead of codecov, technically), but I can't recall conversations about pros & cons myself.

One suggestion is that if we do proceed with this, we might trial it on a smaller Rails project than Gravity to begin with, to get a better idea if we should adopt more widely.

Actually now that I'm poking around I see that Exchange used to have this enabled but it seems like it was turned off at some point? It might be fruitful to ask that team why.

lidimayra commented 2 years ago

Hmm, that's a very good point!! As I understand, Exchange is mostly maintained by @artsy/transact-devs and @artsy/px-devs, right? Would you have some thoughts to be shared on this?

joeyAghion commented 2 years ago

I for one would be very interested in coverage data for a project like Gravity, since it has decent coverage but possibly a lot of slow and redundant tests. We need to constantly be consolidating and refining its tests to avoid excessive bloat.

However, I've been burned by the Codecov integration too many times to advocate for adopting it across more projects:

There was a massive exploit recently in which the Codecov integration allowed malicious code to be executed with access to source code and CI configuration, and necessitated a painstaking review of dozens of repositories' histories for anything sensitive that may have been exposed.
Exchange's CI is running codecov right now, but the artifacts aren't being exposed correctly.
Positron's Codecov integration is 2 major versions behind and tests are failing.

I guess all I'm saying is that these integrations tend to require a lot of maintenance, and seeing value in the results is not the same as volunteering to fix the inevitable issues that come up. I'd be interested to find a way to generate a coverage report periodically rather than integrate them into every CI run, especially if we don't plan to respect the output on a PR-by-PR basis.

lidimayra commented 2 years ago

Great inputs, thanks a lot, @joeyAghion !!

Based on these points, I would think if it would make sense to discuss setting up just simplecov as a code coverage analysis tool (without necessarily integrating it with codecov). What would you think?

joeyAghion commented 2 years ago

I'm not that familiar with simplecov or how that would make the integration simpler. Assuming that's the case, though, I'd be open to it. I second @anandaroop's suggestion to pilot with a smaller Rails project first (like maybe by fixing/replacing Exchange's tooling).

lidimayra commented 2 years ago

In that case, would it make sense to think about this as a Future Friday project?

lidimayra commented 2 years ago

Resolution

We decided codecov might not be the ideal option, but we would like to have coverage data somehow. We'll work on an initial setup for Exchange as a Future Friday project

Level of Support

5: Unclear Resolution.

Additional Context:

Depending on the results of the FF implementation, we'll have more clarity on what would be the best approach for us

Next Steps

Future Friday project

artsy / README