feat: detect QLTY_COVERAGE_TOKEN env var

[brynary]

Hello and thank you for SimpleCov!

This PR updates the detection for the CC_TEST_REPORTER_ID environment variable to also detect the QLTY_COVERAGE_TOKEN env var.

Context: As of this year, the Code Climate Quality product has been replaced by the newer Qlty Cloud edition. (Qlty Cloud is operated by the same team previously responsible for Code Climate Quality.)

Qlty Cloud uses the QLTY_COVERAGE_TOKEN env var in the same way that Code Climate Quality used CC_TEST_REPORTER_ID. This PR ensures JSON code coverage data is automatically generated when it is present.

We discovered that some customers are not aware of the CC_TEST_REPORTER_ID behavior and when they switched from CC_TEST_REPORTER_ID to QLTY_COVERAGE_TOKEN it was confusing that the JSON went missing.

This PR also preserves the CC_TEST_REPORTER_ID behavior for backwards compatibility, for anyone where it may be a "load bearing" behavior.

Please let me know if you have any questions about this change!

-Bryan

[sferik]

In my view, this illustrates why it was probably a mistake to hardcode hooks for a specific commercial product. I would sooner remove support for CC_TEST_REPORTER_ID—backward compatibility be damned—than hardcode another commercial product into this repo. My 2¢.

[brynary]

Hi Erik -- Hope you're well!

I don't have a strong opinion on the core question -- I actually didn't fully realize that this hook was in simplecov-ruby until we had users experiencing breakages.

If there's anything we can do to make supporting this compatibility easier, we would be happy to do that.

My primary concern is breakages for users. (We do offer a commercial product, and we also provide free services to OSS projects who often have less maintenance resources available to them.)

Cheers,

-Bryan

[sferik]

Looking at the history of this feature, it appears to have come from codeclimate-community/simplecov_json_formatter, which explains how the CC_TEST_REPORTER_ID made it into this codebase. Again, I think that was a mistake, but it’s ancient history at this point.

My preferred approach going forward would be deprecating support for that environment variable (with a warning message) in favor of a generic environment variable (something like SIMPLECOV_JSON_FORMATTER) that could be set in any vendor’s code coverage environment to trigger the JSON formatter.

@brynary Would this solve the problem for your customers? I understand it would require them setting two environment variables instead of just one, but maybe QLTY could set this variable by default in Ruby projects that bundle simplecov? I’m open to other creative solutions to this problem that don’t involve checking for a vendor-specific environment variable.

[brynary]

Ok, booting back up on this, thanks for the patience. This is my best understanding...

Over-simplifying a bit, but basically the way people setup code coverage reporting in a CI setting has historically been along the lines of:

# CI workflow pseudo code
env:
  CC_TEST_REPORTER_ID: "random_string" # Or now QLTY_COVERAGE_TOKEN 
steps:
  run: rake test
  run: upload_coverage_data

There are ~tens of thousands of CI workflows checked into version control across private and open source projects. Unfortunately, we don't have the ability to modify them.

Critically, we don't have a hook to be able to set environment variables for the rake test step. That is entirely up to the user's CI config.

So today, simplecov-ruby will see the presence of CC_TEST_REPORTER_ID and will activate a special behavior to write out coverage data in JSON format. That JSON coverage data is then used by a upload_coverage_data step (and potentially other steps).

If rake test stops detecting CC_TEST_REPORTER_ID and using it as a trigger to write out the JSON coverage, then the upload_coverage_data will crash and fail their builds.

This surfaced to us because a customer replaced CC_TEST_REPORTER_ID with QLTY_COVERAGE_TOKEN and their builds broke and they didn't understand why. The answer was they had been relying on this load bearing behavior and the fix involved making sure their rake test was adjusted to still write out JSON.

Given the above, if simplecov-ruby stopped recognizing CC_TEST_REPORTER_ID as a trigger to write out JSON, I would expect user's CI workflows will break when they upgrade simplecov-ruby for reasons that are not obvious. (A warning and transition period would certainly be better than a hard break, but in our experience, warnings in CI logs often go unnoticed or unaddressed until builds start failing.)

[sferik]

N.b. I did not propose removing the CC_TEST_REPORTER_ID behavior; I proposed deprecating it with a warning. Maybe five years from now, you’ll be able to tell me how many of your customers still rely on that variable and we can decide whether to remove it. What I don’t want to do is add another vendor-specific environment variable that we’ll need to support for 10 or more years, as you have proposed in this pull request.

I am proposing your users migrate to something like this:

env:
  QLTY_COVERAGE_TOKEN: "random string"
  SIMPLECOV_JSON_FORMATTER: "true"
steps:
  run: rake test
  run: upload_coverage_data

I imagine that the upload_coverage_data script in this scenario could detect customers still using CC_TEST_REPORTER_ID without SIMPLECOV_JSON_FORMATTER and alert them (e.g. via email) that their setup is deprecated. I understand that doing this would probably require users to update their upload_coverage_data script, which could take some time to roll out, but I assume you have some ability to push software updates to your customers.

Alternatively, your users could do something like this, before calling SimpleCov.start.

if ENV["CC_TEST_REPORTER_ID"] || ENV["QLTY_COVERAGE_TOKEN"]
  SimpleCov.formatter = SimpleCov::Formatter::JSONFormatter
else
  SimpleCov.formatter = SimpleCov::Formatter::HTMLFormatter
end

I suspect something like this is what your competitors require of their customers.

Another possible approach is if upload_coverage_data could parse HTML-formatted reports (in addition to JSON-formatted reports). Obviously, this would be more brittle, but on the plus side, you could do it without needing any changes to this library.

[brynary]

n.b. I did not propose removing the CC_TEST_REPORTER_ID behavior; I proposed deprecating it with a warning. Maybe five years from now, you’ll be able to tell me how many of your customers still rely on that variable and we can decide whether to remove it.

Makes sense, and thanks for clarifying! I was just booting back into this and trying to sort through what the impact would be if/when a deprecation became a removal.

What I don’t want to do is add another vendor-specific environment variable that we’ll need to support for 10 or more years, as you have proposed in this pull request.

I think that's a reasonable approach! Do you feel like this kind of enabling behavior on vendor-specific environment variables has caused issues for simplecov users or maintainers?

I imagine that the upload_coverage_data script in this scenario could detect customers still using CC_TEST_REPORTER_ID without SIMPLECOV_JSON_FORMATTER and alert them (e.g. via email) that their setup is deprecated. I understand that doing this would probably require users to update their upload_coverage_data script, which could take some time to roll out, but I assume you have some ability to push software updates to your customers.

Unfortunately I don't think we could do this because there are many non-simplecov cases where someone sets CC_TEST_REPORTER_ID. By the time upload_coverage_data runs, there may be data in many forms from any number of coverage tools, so we can't really make any confident assertions about the source of that data. Also, since env vars can be assigned on a per-step basis, if upload_coverage_data does not see SIMPLECOV_JSON_FORMATTER, we can't be sure whether the rake test step saw it.

Alternatively, your users could do something like this, before calling SimpleCov.start.
if ENV["CC_TEST_REPORTER_ID"] || ENV["QLTY_COVERAGE_TOKEN"]
SimpleCov.formatter = SimpleCov::Formatter::JSONFormatter
else
SimpleCov.formatter = SimpleCov::Formatter::HTMLFormatter
end

Yes, we can update our setup procedures to reflect this.

Another possible approach is if upload_coverage_data could parse HTML-formatted reports (in addition to JSON-formatted reports). Obviously, this would be more brittle, but on the plus side, you could do it without needing any changes to this library.

Yes, I think this would create headaches for everyone because an HTML change would risk becoming a breaking change. The JSON format helps avoid incidental wire breakages as it's inherently more of a contract than HTML.

As far as alternative ideas, one random idea would be: Would it be reasonable is Simplecov generated JSON by default? I'm not prepared to recommend that, but some coverage generation tools will output a machine-readable coverage report (e.g. a *.lcov file) by default, so thinking about if that may be a way to sidestep.

[brynary]

(Just split out the docs-only part of this to #1162 for convenience.)

[sferik]

(Just split out the docs-only part of this to #1162 for convenience.)

Merged!

What I don’t want to do is add another vendor-specific environment variable that we’ll need to support for 10 or more years, as you have proposed in this pull request.

I think that's a reasonable approach! Do you feel like this kind of enabling behavior on vendor-specific environment variables has caused issues for simplecov users or maintainers?

No, it hasn’t caused a significant maintenance burden, but if one (or two or five) of your competitors open similar pull requests, I want to be able to credibly deny them without looking like I’m showing favoritism to you, Bryan, who I know and like. 😄 Think of it like defensive programming for GitHub Issues. It’s not a problem now, but I have a clear idea of how it could become a problem and am trying to nip it in the bud.

As far as alternative ideas, one random idea would be: Would it be reasonable is Simplecov generated JSON by default? I'm not prepared to recommend that, but some coverage generation tools will output a machine-readable coverage report (e.g. a *.lcov file) by default, so thinking about if that may be a way to sidestep.

I need to think a bit more about this, but on first thought, I like this idea! Two options I’d consider:

1. Switch on $stdout.tty?, so users running SimpleCov in a terminal would get HTML output by default and headless CI runs would get JSON output by default. My guess is that this is what 99% of people would want and expect but I’m sure there are some edge cases where this would break things and/or cause confusion.
2. Always output both HTML and JSON by default. The only real downside to this is that it’s somewhat slower to generate both and creates more "garbage" files that need to be cleaned up later. That said, users who are especially concerned about performance could easily override this default.

Curious if you have a preference between either of these two options (or perhaps you had a third one in mind).

[PragTob]

@brynary @sferik

Alright here it goes. I will preface this by saying that @sferik did the recent work here, and so it's his call.
I will also say, that it was my plan to remove this environment variable for simplecov 1.0 and push code climate to be explicit about running formatters. And I will say, that my interactions with code climate around this topic have been extremely frustrating and majorly contributed to me stepping away from maintaining simplecov, as I didn't want to deal with this any more.

Historical Context

• I don't know when, but code climate started relying on .resultset.json which was an undocumented implementation detail file we produced as a side effect of merging multiple coverage runs through parallel_tests. It included the raw coverage data (no filtering, no ignores) so was also wrong. To the best of my knowledge no one ever asked simplecov if this was good/safe to rely on (it wasn't). I also consider the fact that this was file was produced without a parallel test run a bug.
• At some point we changed the format, moved the file, I don't remember. That caused code climate to break.
• I suggested to the code climate team to please write their own wrapper or instruct their users to configure a formatter, all of these attempts were absolutely stone walled and denied. It had to work out of the box with no adjustments for code climate users ("just run simplecov")
• The compromise I ended up with them, after a lot of discussions and even a call, was: Code climate maintains the JSON formatter so we don't add more maintenance overhead to an already overloaded project that is hard to maintain. Also, we'll only run the formatter in an environment we know is yours, to not run unnecessary code for other users because you refuse to let your users configure something - that's where the env came from.
  • I was never remotely happy about this compromise, but even in a call saw absolutely no flexibility from the code climate team and so decided to go with this, as the ones suffering are ultimately their users which felt unfair

Merging the JSON formatter means more maintenance for any current or future maintainers of simplecov. Running it for everyone means we spend resources on everyone's machine. Because a company didn't want to add setup instructions for using their tool.

Why are we doing this?

From where I'm standing we're doing this essentially because a company refused to add setup instructions for how to use their product. And now we've further pushed the maintenance burden of features onto unpaid volunteers, to support the functioning of a money making product.

This all started because code climate started relying on an undocumented implementation detail, never meant for consumption, without collaborating with the open source team.

What could code climate have done instead?

• have a simple setup instruction to please add any Code Climate provided formatter to their simplecov config
• write a small wrapper around simplecov, making sure defaults such as the formatter are set and maybe other stuff
• I would have even been open to the idea of reading an environment variable on the simplecov side like (SIMPLECOV_FORMATTERS or something) to configure formatters to make it easier to configure that JSON output happens
  • this would also require to at least depend on the JSON formatter though, which was already bad imo

This whole also sets a dangerous precedent, as Erik mentioned above. Why are we doing things to cater to one specific provider? What if another asks for stuff? What if the requirements conflict? Why are we taking on work (the JSON formatter) that as best as I know was only asked for for this? Why spend my volunteer time on this?

The interactions I've had with code climate on this have been the anti-thesis of how I believe companies should collaborate with open source. There wasn't collaboration, but there was pushing work on open source projects - who are already under staffed (as you can see with the amount of issues simplecov has to deal with). I say this as someone, who used recommend people to use code climate.

Why it's bad to run the JSON formatter by default

First, I care about performance - no code runs faster than no code. You may benchmark it and say it's not that much, you may be right but multiply it by all the people who run simplecov locally or on CI every day (and some huge code bases like f.ex. Shopify push surprising limits there). Huge waste of time and energy.
And what also matters, I think as OSS maintainers we also have a responsibility to our users - and part of that responsibility (imo) is not run unnecessary code.

Second, the not run unnecessary code part is not just due to performance but also due to stability. All code has bugs, maybe even crashes. More code run = more possible crashes. Security wise, it also means more possible exploits. So, do we want to potentially fail people's CI for a feature they never asked for or wanted?

Now there are common counter arguments:

1. "People can just deactivate it" - Defaults matter. We make this the default to support one specific provider due to their unwillingness to change. This is a small percentage of our user base. We waste everyone else's resources - why? Why should the many/default make the change and not the small percentage of our user bases? Opt-in vs. Opt-out.
2. "The HTML formatter could use the output and hence it would be necessary" - It's an intriguing idea but ultimately without too much merit, imo. Right now we already have the coverage data in nice Ruby objects and use that to generate the HTML and JSON. Outputting the JSON, parsing it back to Ruby to generate the HTML is just extra steps vs. using the Ruby objects we already have. Now of course HTML/JS could do the parsing but that's a different formatter then. If there was a nice common HTML/JS code coverage formatter that we could use, removing the burden to maintain our own, that relied on a JSON format being present I think I could be convinced that it'd be better for the maintenance team.

Summary

I'd strongly prefer for code climate to update their instructions to configure the JSON formatter to run vs. us running it by default. This should be opt-in.
I'd also prefer for the JSON formatter not to be merged into simplecov. The only need I ever recall seeing for it was code climate. I don't see why we should add to the already big maintenance burden of simplecov. If we did, we should make sure that we as simplecov maintainers want it and want to maintain it going forward because we think it is important for all simplecov users. Personally, I think it's perfectly fine as a separate gem and I'd welcome the contribution of code climate to the eco system by maintaining it.

But, I'm also saying that I don't think I have a huge say in this through my inactivity in recent years. I did want to provide my input and historical context though, as well as thoughts about how companies work with and use OSS.

[sferik]

@PragTob thank you for writing this all out. This history is genuinely useful, and I want to start by acknowledging how much of SimpleCov exists because of your work. You carried this project for years, and the fact that I'm even in a position to propose changes to it is because you kept the lights on.

I agree that the “original sin” was taking on simplecov_json_formatter as a dependency. Once we did that, it’s bugs became our bugs. Unfortunately, it hasn’t been actively maintained for years, so we’ve been carrying the maintenance burden without the ability to fix things cleanly. Merging it gives us the ability to do that and I’m happy to take on that responsibility going forward.

Outputting the JSON, parsing it back to Ruby to generate the HTML is just extra steps vs. using the Ruby objects we already have.

I’m not going to address your post point-by-point, but on this concern specifically, I think it’s worth clarifying, because that’s not what #1165 does. The JSON is never parsed back into Ruby. The two formatters share a single JSONFormatter.build_hash(result) entry point that produces a hash. The JSON formatter serializes it to coverage.json and the HTML formatter serializes the same hash to a coverage_data.js file that the browser reads as window.SIMPLECOV_DATA. All ERB, Ruby view helpers, and server-side source rendering have been deleted (about 2,700 lines removed). The HTML formatter is now effectively: call build_hash, write two files, and copy static assets. That’s it.

I expect the combined JSON + HTML pipeline in #1165 to be faster than the old HTML-only pipeline—even on very large projects. If you know someone as Shopify or another large Ruby app who is willing to verify this before I merge it, I’m happy to wait for the results.

Independent of anything related to Code Climate/Qlty, I think treating JSON as the data layer and HTML as a view over it is just the right direction for this tool. JSON is the lingua franca for interop. Making coverage.json a first-class, documented, enriched format means any third-party tool can consume a stable contract instead of reverse-engineering .resultset.json. This seems like the right direction for this project and will allow us to remove CC_TEST_REPORTER_ID, which I just added to that PR.

I came into this without any of the historical context you shared, and I appreciate you taking the time to write it down rather than just letting the frustration sit. My hope is that, with the formatter merged and owned by us, and with a documented JSON contract that any vendor can target, we can bury the hatchet and move forward as a community. Any future vendor gets the same deal: target the public coverage.json contract (or ship their own formatter).

Thanks again for chiming in. Your input carries a lot of weight with me personally and it’s important to me to evolve this project with your blessing.

[brynary]

Hi @PragTob -- (Code Climate founder here.) I just wanted to chime and and convey an apology for Code Climate inflicting additional maintenance burden on you and SimpleCov.

The history spans over a decade at this point, and across a period of time when I was not personally involved in that engineering work, so I don't have direct exposure to the specifics, but based on your description the fact that it was a negative impact is apparent to me now. (And was not known to me personally, prior to today, unfortunately.)

I imagine a well-intentioned engineer saw the .resultset.json as an output and built a processor for that data without considering all of the implications. Obviously, the right integration mechanism is through an interface that is intended to be stable. That could either be a programmatic interface (like a formatter plugin, which I believe SimpleCov has had for a long time) or a data interface.

I was not a party to the conversation you mentioned about how to evolve past that original sin, which sounds like an additional layer of frustration on top of a problem we created.

Across all programming languages, test coverage collection setup has historically (especially pre-AI) been a challenge for our users to get right -- That's not a deficiency of coverage tools, just a challenge of getting things right in varied and complex CI environments. I could imagine (hope) that the pushback you felt was driven by I-would-assume-well-intentioned-but-potentially-myopic focus on avoiding breakages for customers and OSS projects who had gone through the setup process (which number in the ~10ks), but that's essentially speculation. We clearly could have and should have done better working through this with you as well.

Thank you for your work on SimpleCov, as it is a vital component of the Ruby ecosystem.

[PragTob]

@sferik

I agree that the “original sin” was taking on simplecov_json_formatter

I wouldn't say that, to me the original sin was someone relying on .resultset.json and refusing to add documentation to their setup, forcing the hand in then including the formatter. I could have refused but that didn't feel right/workable at the time.

Once we did that, it’s bugs became our bugs.

Imo not really as it was only run in one extremely specific context. My goal/intent would have been to remove the dependency with 1.0 and let it live as its own project. This works really well for me in my benchee library, where there are a bunch of formatters but a good chunk of them are community maintained.

I think it’s worth clarifying, because that’s not what #1165 does.

Sorry if it felt this was aimed at #1165 - I hadn't looked at it and still haven't done so now. From the title I assumed it was simplifying the interface of formatters or how they were called. I don't wanna discuss that here, let's discuss that in that PR :)

Thanks again for chiming in. Your input carries a lot of weight with me personally and it’s important to me to evolve this project with your blessing.

💚

@brynary

Hey, I know who you are :) And I've also used your OSS before, or if I'm not mistaken I did (I think you made the package that was a precursor to code climate in a way, i.e. combining simplecov, flog, flay and other stuff with a nice HTML report - thanks for that). And while that didn't make it into my post above, I acknowledge and appreciate seeing you around some of the PRs (also, like this one) helping.

Thanks for the apology, although it was not your fault.

If you wanted to dig into the history most of it (minus the emails and meeting I think) is in here: qltysh-archive/test-reporter#413 - I'm only posting it for reference. It's been a long time and I don't think there's any need to dig into it.

Across all programming languages, test coverage collection setup has historically (especially pre-AI) been a challenge for our users to get right

tell me about it 😅 Before I started maintaining simplecov I thought there were too many mentions of "make sure simplecov is required and started before anything else" in the README. A couple of months in I added even more.
People just want code coverage to work and so there are a lot of vague issue reports to deal with, making this hard. And some of them also for services (the above issue I linked started with an issue report of my to code climate, as I was getting reports from code climate users in simplecov).

I could imagine (hope) that the pushback you felt was driven by I-would-assume-well-intentioned-but-potentially-myopic focus on avoiding breakages for customers and OSS projects who had gone through the setup process (which number in the ~10ks), but that's essentially speculation.

They were definitely well intentioned and nice people. Alas, unwilling to make a couple of key compromises that I believe would have ultimately been beneficial for all sides.