rspec:undercoverage job can be unreliable due to simplecov results (minimal test runs)
Problem
It has been noticed that the rspec:undercoverage job on GitLab CI runs can give unpredictable results.
When code is covered by tests, sometimes it passes, and sometimes not.
Examples
-
From !87714 (merged) we see it failing in https://gitlab.com/gitlab-org/gitlab/-/jobs/2480187016, but coverage exists and a new pipeline passes.
- downloading the coverage files from the coverage job in that same pipeline shows that simplecov results are errant. Screenshot below of one of the lines that undercoverage indicates isn't covered.
-
A similar issue can be seen in this MR
Hypothesis
SimpleCov results are incomplete sometimes.
SimpleCov results are incomplete because minimal test runs do not run the relevant tests.
Minimal test runs do not run the relevant tests because a file was re-named.
slack thread
Gonna start a new thread for this: anyone got any ideas on how to solve this? https://gitlab.com/gitlab-org/gitlab/-/issues/357381
Summary: if you wanna test the final output of the component, simplecov has no idea what methods it touched, so code coverage stats are rubbish even though you've covered everything in the tests. Means you either have to ignore whole blocks of code in simplecov or you have to write all the tests twice - once to test the view, once in a unit test for each method :see_no_evil:
GitLabGitLab
Undercoverage reports lack of coverage in ViewComponent classes (#357381) · Issues · GitLab.org / GitLab · GitLab
From !83882. A method and its...
Author
Robert May
67 replies
leipert 5 hours ago
once to test the view, once in a unit test for each method
Do you mean each method of the component and once for the actual page view?
Robert May 5 hours ago
Yeah. So for interface stuff you usually wanna test stuff like "did this piece of text get rendered on the page", but if you do that, simplecov sits there like "no tests hit this method that returns this piece of text" even though you literally checked it was on the page
leipert 5 hours ago
Because then this might be desired. Unit test everything in them in isolation and have some integration tests?
Unlike traditional Rails views, ViewComponents can be unit-tested. In the GitHub codebase, component unit tests take around 25 milliseconds each, compared to about six seconds for controller tests.
leipert 5 hours ago
basically this is hitting similar problems that we have with our Vue apps / components.
leipert 5 hours ago
We also get no coverage from "normal" rspec tests and need to write separate unit tests.
You might have components where it is okay to have no coverage, and ideally your normal rspec controller/integration tests should really just test the result and not care whether it uses a ViewComponent or Voodoo under the hood?
Robert May 5 hours ago
Some examples of methods I ended up ignoring:
https://gitlab.com/gitlab-org/gitlab/-/issues/357381
https://gitlab.com/gitlab-org/gitlab/-/merge_requests/83970/diffs <- ignored the whole component cos I was getting annoyed :laughing:
GitLabGitLab
Migrate diffs overflow warning to component (!83970) · Merge requests · GitLab.org / GitLab · GitLab
What does this MR do and why? Describe in detail what your merge request does and why. Moves the...
Author
Robert May
Assignee
Robert May
Robert May 5 hours ago
Yeah it's a bit awkward - I kinda like to have a hybrid approach where most of my tests are on the final result, but with unit tests to cover certain methods that need more testing
Doug Stull 3 hours ago
with ViewComponents, it is suggested we test behavior(https://viewcomponent.org/viewcomponents-in-practice.html#prefer-tests-against-rendered-content-not-instance-methods) not the actual instance methods and I also tent to leave everything private in them, if possible - https://viewcomponent.org/viewcomponents-in-practice.html#most-viewcomponent-instance-methods-can-be-private. I too found we have to # :nocov: them a lot for undercoverage
ViewComponentViewComponent
ViewComponents in practice
A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
ViewComponentViewComponent
ViewComponents in practice
A framework for building reusable, testable & encapsulated view components in Ruby on Rails.
Doug Stull 3 hours ago
also added to https://gitlab.com/gitlab-org/gitlab/-/issues/357381
GitLabGitLab
Undercoverage reports lack of coverage in ViewComponent classes (#357381) · Issues · GitLab.org / GitLab · GitLab
From !83882. A method and its...
Author
Robert May
Doug Stull 3 hours ago
I would really like to stick with the testing behavior concepts and keeping things private. If undercoverage/simplecov doesn't like that, then I'm not sure how that can be resolved currently
:+1:
1
Doug Stull 3 hours ago
I have also found undercoverage flakey in reporting - this passing job https://gitlab.com/gitlab-org/gitlab/-/jobs/2484822800 failed on previous pipeline in same MR with no other changes https://gitlab.com/gitlab-org/gitlab/-/jobs/2480187016
GitLabGitLab
rspec:coverage (#2484822800) · Jobs · GitLab.org / GitLab · GitLab
GitLab is an open source end-to-end software development platform with built-in version control, issue tracking, code review, CI/CD, and more. Self-host GitLab on your own servers, in a...
GitLabGitLab
rspec:undercoverage (#2480187016) · Jobs · GitLab.org / GitLab · GitLab
GitLab is an open source end-to-end software development platform with built-in version control, issue tracking, code review, CI/CD, and more. Self-host GitLab on your own servers, in a...
Doug Stull 3 hours ago
@Jeremy Jackson I believe saw this unrelated-to-viewcomponent-undercoverage weirdness across diff pipelines in same MR as well in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/86700#note_953318208
GitLabGitLab
Adds product interaction data to leads (!86700) · Merge requests · GitLab.org / GitLab · GitLab
Closes #354269, and...
Author
Jeremy Jackson
Assignee
Jeremy Jackson
Only visible to you
Slackbot 3 hours ago
Ok! I've sent @Jeremy Jackson a link to your message.
Robert May 3 hours ago
Oh I'm so glad it isn't just me running into this :laughing: Thought it might have just been my way of testing
:100:
1
Doug Stull 3 hours ago
you aren't alone :slightly_smiling_face:
gabriel 3 hours ago
Some more complex components can benefit from some unit tests, but I agree behaviour tests are the way
gabriel 1 hour ago
question: is the coverage a problem only with undercoverage or does it fail with simplecov as well?
Doug Stull 1 hour ago
I was going to look into that...but ran out of time
gabriel 1 hour ago
I have a gut feeling that simplecov is fine and undercoverage is what is broken… I’ve seen this in the past where simplecov parsed results inside rubymine was ok but undercoverage was freaking out
Doug Stull 1 hour ago
the non-viewcomponent case that @Jeremy Jackson hit with his MR seemed to prove out that simplecov saw the coverage when ran locally - so there was something between that and the merging or analyzing of those results that was having an issue
:+1:
1
gabriel 1 hour ago
could it be due to haml? :thinking_face:
Doug Stull 1 hour ago
i'm going to run my case locally for view component and see what it comes up with
gabriel 1 hour ago
would be worth testing out a non haml case, maybe it could just be lack of including certain file extensions or something like that (edited)
Doug Stull 1 hour ago
what do you mean by 'non haml case'? is that where we aren't focused on render_inline?
Doug Stull 1 hour ago
or do you mean erb instead?
gabriel 1 hour ago
ERB
gabriel 1 hour ago
it just a long guess that undercoverage may not handle haml correctly (edited)
Doug Stull 1 hour ago
not a bad guess - running SIMPLECOV=1 be rspec ee/spec/components/namespaces/free_user_cap/alert_component_spec.rb locally now from https://gitlab.com/gitlab-org/gitlab/-/jobs/2480187016#L102
gabriel 1 hour ago
if we found that is the case it’s easier to look for a solution
Doug Stull 1 hour ago
simplecov sees coverage locally on https://gitlab.com/gitlab-org/gitlab/-/jobs/2480187016#L104
Doug Stull 1 hour ago
my run just finished
Doug Stull 1 hour ago
this lines up with some of what @Jeremy Jackson was seeing with non-viewcomponent
Doug Stull 1 hour ago
starts to look like more of an issue between simplecov results and undercoverage/how we use it
Doug Stull 1 hour ago
coverage is found...just undercoverage job reports it incorrectly...sometimes
gabriel 1 hour ago
could it be due to ee/ folder ?
:thinking_face: (edited)
Doug Stull 1 hour ago
maybe? that might suggest a merging of results issue?
gabriel 1 hour ago
either that or it doesn’t understand having multiple “roots”
gabriel 1 hour ago
but likely an issue with merging (due to that)
Doug Stull 1 hour ago
simplecov results for proof on https://gitlab.com/gitlab-org/gitlab/-/jobs/2480187016#L104 to round that out
Screen Shot 2022-05-20 at 9.54.58 AM.png
Screen Shot 2022-05-20 at 9.54.58 AM.png
Doug Stull 1 hour ago
we need undercoverage expert
gabriel 30 minutes ago
I guess the people behind it is available for hire / support contract :troll:
gabriel 28 minutes ago
https://undercover-ci.com/ we could buy them as well, etc :moneybag:
undercover-ci.com
UndercoverCI | Actionable test coverage checks for Ruby and Github
Stop focusing on getting to 100% test coverage. Reduce pull request defects by telling when changed code is untested before it's deployed to production.
:100:
1
Doug Stull 28 minutes ago
from the failed undercoverage job that I previously mentioned - I went to the coverage job and looking at the simplecov results for that alert_component.rb file, it seems like it isn't showing the coverage https://gitlab-org.gitlab.io/-/gitlab/-/jobs/2480187013/artifacts/coverage/index.html#_AllFiles - so this looks like an issue with our simplecov runs firstly here
Doug Stull 27 minutes ago
or the merging of the simplecov data - but I am thinking it is less undercoverage issue and more simplecov reporting now
:thinking_face:
1
gabriel 26 minutes ago
we can have the merge job render simplecov as html and export it as artifact to validate that (edited)
Doug Stull 26 minutes ago
download the files from https://gitlab.com/gitlab-org/gitlab/-/jobs/2480187013 to validate
Doug Stull 26 minutes ago
yes - that is what you can get from the above job
gabriel 25 minutes ago
I wonder if we missed some steps here: https://undercover-ci.com/docs#parallel-tests
undercover-ci.com
UndercoverCI | Documentation
Learn how to install UndercoverCI for your repos, report coverage data and use the local CLI.
Doug Stull 24 minutes ago
Screen Shot 2022-05-20 at 10.32.47 AM.png
Screen Shot 2022-05-20 at 10.32.47 AM.png
gabriel 23 minutes ago
:thinking_face:
gabriel 23 minutes ago
ok that makes sense, we have a problem with merging for simplecov, this makes it easier to trackdown at least
Doug Stull 23 minutes ago
scripts/merge-simplecov is our merge file
gabriel 22 minutes ago
we used to have a custom code, but we now rely on simplecov internal logic
gabriel 22 minutes ago
I know this because I’ve updated this file and upgraded simplecov a while ago :troll:
:evil:
1
gabriel 22 minutes ago
it is still not 100% I guess
gabriel 21 minutes ago
when I did we had issues with coverage as well, so I was expecting it to fix it
gabriel 21 minutes ago
:confused:
Doug Stull 20 minutes ago
the essence of
merged_result = SimpleCov::ResultMerger.merge_results(*results)
from https://undercover-ci.com/docs#parallel-tests seems to be captured in the collate command in our merge-simplecov script
:+1:
1
gabriel 19 minutes ago
the new version added a few branch cases it didn’t catch before
:thinking_face:
1
Doug Stull 18 minutes ago
yeah - was ok to not work 100% so much when we didn't block CI pipelines on it...now it feels like more of an issue
Doug Stull 18 minutes ago
i don't like using pipeline:skip-undercoverage ...but
New
gabriel 11 minutes ago
@Doug Stull as you have put some effort in identifying the root cause, would be useful to have an issue so we don’t forget this again or can resume the investigation
gabriel 10 minutes ago
what we learned here so far is that there is a problem with merging coverage data and that is in simplecov
Doug Stull 10 minutes ago
I was thinking the same thing...
Doug Stull 10 minutes ago
i'll open an issue - one moment
gabriel 10 minutes ago
I guess the next step would be to try to make a “reproducible repo” and open an issue upstream somehow
gabriel 9 minutes ago
considering this reality we may want to consider disabling undercover for this part of the codebase if it’s not working
Doug Stull 8 minutes ago
the issue is 'which part' - i can only say i've seen it flakey in a few areas...but that could be observation bias as those were the areas I am in/was reviewingEdited by Thong Kuah