tests: Ensure all core files are required #48250

bfontaine · 2016-01-19T14:44:47Z

This is a work in progress; I’m still not able to locally get the same coverage as Travis. cc @UniqMartin

MikeMcQuaid · 2016-01-19T14:58:08Z

Library/Homebrew/test/testing_env.rb

+      # scripts
+      "build.rb", "postinstall.rb", "test.rb",
+      # mocked files
+      "config.rb", "global.rb",


I think it would still be good to work out a way of getting these all included and/or put them into designation directories.

What do you mean by “designation directories”? We could e.g. mark them as non-coverable with # :nocov:.

Sorry, I mean having these files in directories that indicates they are e.g. scripts/configuration/etc.

bfontaine · 2016-01-19T15:46:22Z

I killed the build; no need to waste CI time on this for now.

UniqMartin · 2016-01-19T15:48:01Z

May I suggest a slightly modified version? It makes the path checks much easier.

if ENV["HOMEBREW_TESTS_COVERAGE"]
  # Ensure all Ruby files are `require`d for an accurate coverage measurement
  library_base = File.expand_path("../..", __FILE__)
  Dir["#{library_base}/**/*.rb"].each do |f|
    f_rel = f[(library_base.length + 1)..-1] # path relative to `library_base`
    next if %w[build.rb postinstall.rb test.rb].include?(f_rel) # scripts
    next if %w[config.rb global.rb].include?(f_rel) # mocked files
    next if f_rel == "cmd/tests.rb" || f_rel.start_with?("test/") # test files

    require f
  end
end

bfontaine · 2016-01-19T15:48:52Z

@UniqMartin What is the secret of the low coverage? 😄

MikeMcQuaid · 2016-01-19T15:55:32Z

👍 to @UniqMartin's version.

UniqMartin · 2016-01-19T15:58:04Z

What is the secret of the low coverage? 😄

The issue is with Coveralls and the way we include it in our .simplecov, which overwrites the default HTML output formatter of SimpleCov, but then outputs nothing because it isn't running within CI (when we do local testing). The only reason we still get an output to index.html in this scenario is because while running the integration tests we do not enable Coveralls and thus get an output (and the coverage) from when the last integration test finishes.

The results cache is only updated with the final results after the last unit test finishes, but with Coveralls enabled index.html isn't regenerated. Because this cache is reused on a second run, it now contains the results from the previous unit test run and the coverage is much higher.

This is what fixed it for me locally and should continue to work on Travis:

diff --git i/Library/Homebrew/test/.simplecov w/Library/Homebrew/test/.simplecov
index 34d22184..e23a1256 100644
--- i/Library/Homebrew/test/.simplecov
+++ w/Library/Homebrew/test/.simplecov
@@ -24,7 +24,9 @@ if name = ENV["HOMEBREW_INTEGRATION_TEST"]
   end
 end

-if RUBY_VERSION.split(".").first.to_i >= 2 && !ENV["HOMEBREW_INTEGRATION_TEST"]
+# Don't use Coveralls outside of CI, as it will override SimpleCov's default
+# formatter causing the `index.html` not to be written once all tests finish.
+if RUBY_VERSION.split(".").first.to_i >= 2 && !ENV["HOMEBREW_INTEGRATION_TEST"] && ENV["CI"]
   require "coveralls"
   Coveralls.wear!
 end

UniqMartin · 2016-01-19T16:01:16Z

The issue became obvious once I tried running brew tests --coverage --verbose TEST=test_ARGV.rb with a clean cache and got no output at all.

bfontaine · 2016-01-19T16:12:13Z

Awesome, thank you @UniqMartin! I looked at the results cache but never thought about Coveralls breaking stuff.

xu-cheng · 2016-01-19T16:19:49Z

Library/Homebrew/test/testing_env.rb

+
+    require f
+  end
+end


We don't need this right?

We need it otherwise some files are missing from the coverage report.

I would like to avoid this. Becuase this feels hack to me and likely to cause trouble: e.g. There may be conflicts, the order of files during require matters.

If it doesn't missed out any coverage. I would be OK to keep the original behavior. If a file is missing from report, we know the coverage for that file is zero right?

It's not a hack, this is commonly done for coverage checking in Ruby projects.

By hack, I mean I rarely see other project to require all files using file glob. (On the hand, manually requiring all the code using require "libname" is quite common. )

My concerns are (a) this can raise hard to debug problem, as the order matters. (b) brew tests might give false postive result, because we don't require all codes like this outside test.

Becuase this feels hack to me and likely to cause trouble: e.g. There may be conflicts, the order of files during require matters.

If this is the case, this feels like a hack to me. Correct me if I'm wrong, because my experience with Ruby is still limited, but we should fix those cases by renaming identifiers and fixing the requires at the top of those files instead of relying on order and only a subset being required.

Is there an easy way to dump all the modules/classes/methods known to Ruby at a certain point during the program runtime? That would help to systematically check for some of those issues.

If it doesn't missed out any coverage.

There are a lot of files we don't test yet. After this change, our coverage is down by about 10%. Not super nice, but at least the new number is much closer to reality.

If it's only required for coverage tests: I think it's fine. Without something like this: our coverage % is completely wrong and we should remove Coveralls. Longer-term if we have every file included somewhere: 👍

I would suggest don't include this even for coverage test. Because, I think to get correct test result is more important than to achieve maximum coverage.

Also it's worthy nothing to mention, even with something like this, the coverage percent is still wrong. We don't count coverage of codes used when running build or postinstall script.

As for coverall, I think it's fine to keep it. Because I know the percent is wrong, at the same time, it does a good job to tracking which lines are already covered(notice to that it doesn't track what lines not covered)

bfontaine · 2016-01-20T00:35:53Z

I added some tests to compensate for the coverage % lost (this is not enough, though).

xu-cheng · 2016-01-20T03:42:13Z

@bfontaine I really think we shouldn't require all the files. Becuase it's pointless to have a coverage report with its test result being incorrect.

Thankfully, after taking look at simplecov API, I found this. http://www.rubydoc.info/gems/simplecov/SimpleCov/Configuration#track_files-instance_method
I think this is right way to get accuracy coverage result without messing with the running code.

UniqMartin · 2016-01-20T03:52:12Z

Thankfully, after taking look at simplecov API, I found this. http://www.rubydoc.info/gems/simplecov/SimpleCov/Configuration#track_files-instance_method
I think this is right way to get accuracy coverage result without messing with the running code.

@xu-cheng In theory, that's the perfect solution and by now I agree with you that we shouldn't simply require all those files. In practice, there are some problems with this suggestion:

track_files requires SimpleCov 0.11.x, but we're using 0.10.0.
We cannot update to SimpleCov 0.11.x as it has a bug with handling root that makes it completely useless to us (I tried, but it seems there's no easy workaround).
track_files is not very well integrated and will partially override coverage information when merging multiple result sets (functionality we rely on for our integration tests).

MikeMcQuaid · 2016-01-20T08:22:35Z

I really think we shouldn't require all the files. Becuase it's pointless to have a coverage report with its test result being incorrect.

The "test result being incorrect" seems to be speculative rather than actual so I'd rather see actual, current tests that fail/pass that shouldn't.

I'd really like to see this require method merged and then iterate if we can find a better method which still produces correct coverage.

UniqMartin · 2016-01-20T08:48:02Z

Library/Homebrew/test/testing_env.rb

+    # mocked files
+    next if %w[config.rb global.rb].include?(f_rel)
+    # test files
+    next if f_rel.start_with?("test/")


I think it would be a good idea to extend this to next if f_rel.start_with?("compat/", "test/", "vendor/") to match the filters we have in .simplecov and because we don't care about the coverage of those files. The comment above would require adjustment for this, too.

Additionally, we should probably exclude compat.rb to avoid pulling them in indirectly. (And because I think we don't want this compatibility stuff to affect testing.)

@UniqMartin shouldn’t we test compat/ files as well?

Maybe. But I guess the add_filter "Homebrew/compat/" and the --no-compat flag of brew tests is there for a reason?

xu-cheng · 2016-01-20T09:44:53Z

The "test result being incorrect" seems to be speculative rather than actual so I'd rather see actual, current tests that fail/pass that shouldn't.

@MikeMcQuaid You can take a look at CI report for this very PR. What you will see is brew tests failed but brew tests --coverage passed. Noted the former one doesn't require all codes in such arbitrary way. It's also important to notice that many codes inside compat folder(and maybe other place) are order sensitive.

still produces correct coverage.

A note regardless this PR: There are in fact many missing coverage when invoking build.rb and postinstall.rb script.

bfontaine · 2016-01-20T09:56:29Z

That sounds hacky but we could maybe require all files in a separate job that doesn’t affect the current one (similarly to integration tests), so that we ensure all files are covered while at the same time running the tests in a clean environment (that is, without require-ing everything)?

bfontaine · 2016-01-20T09:57:02Z

@xu-cheng Good catch, this is indeed an issue that’s due to the require-all-the-things.

bfontaine · 2016-01-20T10:43:55Z

To keep this PR simpler I removed the added tests and made a separate PR instead (#48281).

UniqMartin · 2016-01-20T10:47:27Z

I was able to fix a local copy of SimpleCov 0.11.1 to the point where its new track_files feature probably works as it should. Thus I was able to run brew tests --no-compat --coverage without requireing all the files, but still got coverage results for all 170 relevant Ruby files (the ones not used by any test get a coverage of 0%). The result is not very pretty, but it should be quite accurate: about 42% coverage.

xu-cheng · 2016-01-20T10:51:59Z

@UniqMartin I find a way to workaround the root_filter issue with following code:

SimpleCov.start do
  ...
  filters.clear
  add_filter { |src| !src.start_with?(SimpleCov.root) }
  ...
  track_files "#{SimpleCov.root}/**/*.rb"
end

But I haven't found a way to solve merge issue

UniqMartin · 2016-01-20T11:03:03Z

But I haven't found a way to solve merge issue

Thanks for pointing me at this PR! I somehow failed to recognize it as what it is. My fix requires a relatively simple change to SimpleCov::ArrayMergeHelper#merge_resultset:

diff --git i/lib/simplecov/merge_helpers.rb w/lib/simplecov/merge_helpers.rb
index 339210d1..ca6b8a9f 100644
--- i/lib/simplecov/merge_helpers.rb
+++ w/lib/simplecov/merge_helpers.rb
@@ -2,6 +2,12 @@ module SimpleCov
   module ArrayMergeHelper
     # Merges an array of coverage results with self
     def merge_resultset(array)
+      # Don't merge with all-zero array to preserve nil values from actual runs.
+      if size == array.size
+        return array.dup if all? { |el| el == 0 }
+        return dup if array.all? { |el| el == 0 }
+      end
+
       new_array = dup
       array.each_with_index do |element, i|
         if element.nil? && new_array[i].nil?

(I should probably contribute that upstream, but I first wanted to get that working with our code. And this only happened about 15 minutes ago.)

UniqMartin · 2016-01-21T08:53:08Z

That sounds hacky but we could maybe require all files in a separate job that doesn’t affect the current one (similarly to integration tests), so that we ensure all files are covered while at the same time running the tests in a clean environment (that is, without require-ing everything)?

If you can be bothered to try this, I think this is a good compromise between our desire to get coverage for all our files (excluding scripts for now) while eliminating the undesirable effects require-ing all of them for all other tests has. But I can sympathize if you don't want to waster your time on that.

Because eventually, we should be able to replace this by switching to a newer simplecov and using its track_files feature. But that might take a while, as there are issues that need to be resolved and a version that contains the fixes to be released (we might not want to run a custom-patched simplecov).

MikeMcQuaid · 2016-01-25T08:25:22Z

Because eventually, we should be able to replace this by switching to a newer simplecov and using its track_files feature. But that might take a while, as there are issues that need to be resolved and a version that contains the fixes to be released (we might not want to run a custom-patched simplecov).

I'm 🆗 with us running a patched one as long as the patch is submitted upstream (and ideally accepted).

UniqMartin · 2016-01-26T14:05:03Z

I'm 🆗 with us running a patched one as long as the patch is submitted upstream (and ideally accepted).

How would we go about doing that, i.e., what goes into test/Gemfile and where to put the patched simplecov files then? I'll try to submit my fixes to upstream or to contribute them to the already open PRs soon. I'm just having very limited time right now …

MikeMcQuaid · 2016-01-26T14:10:29Z

@UniqMartin You can point the test/Gemfile to (y)our own fork's branch. Don't worry about the timing, this isn't a huge rush or anything.

bfontaine · 2016-01-26T14:10:59Z

@UniqMartin I can handle that if you feel you don’t have the time.

UniqMartin · 2016-01-26T14:35:01Z

@bfontaine Feel free to do that, if you like. Here's a bit more information about the 0.11.0/1 issues:

The fix for the track_files feature with merging result sets is essentially in the diff in #48250 (comment). The logic behind that fix is: If one of the files from a result set has all zeros, then it's probably generated by the track_files feature and we take the other one (which might or might not be from an actual run, but if it is, using it won't destroy the nil values for empty lines, comment lines, etc.). That should probably go into simplecov-ruby/simplecov#436.

The other issue is with the root_filter being set before a custom .simplecov has the chance to alter root (which defaults to pwd). In our case this will cause all files outside of Library/Homebrew/test/ to be filtered, always resulting in an empty coverage report. That's a regression caused by simplecov-ruby/simplecov#396 and an attempt at a fix is in simplecov-ruby/simplecov#437.

bfontaine · 2016-01-28T00:00:55Z

I believe I’ve now a working solution. I disabled track_files for integration tests to speed up the test suite (70 seconds on my mac vs. 3m51s) and had to make a few tweaks to have everything working with the patched simplecov.

We’re now down at ~46% coverage, which is quite bad but at least it’s now the correct number 👍

Update 2016/02/03: now at ~53%.

UniqMartin · 2016-01-28T11:04:21Z

Library/Homebrew/test/.simplecov

  coverage_dir File.expand_path("#{tests_path}/coverage")
  root File.expand_path("#{tests_path}/../../")

  add_filter "Formula/"
+  add_filter "Taps/"


Any reason not to put this in alphabetical order?

UniqMartin · 2016-01-28T11:09:19Z

Thanks for all the work and for transforming the hints I left into a working solution! I really like the outcome! 😸 👍

I disabled track_files for integration tests to speed up the test suite (70 seconds on my mac vs. 3m51s) and […]

Cool! Simple change but massive impact. The slow-down caused by reading and writing megabytes of JSON over and over again is even more noticeable than I thought.

bfontaine · 2016-01-28T11:11:04Z

The slow-down caused by reading and writing megabytes of JSON over and over again is even more noticeable than I thought.

Before I noticed it was also tracking files under Taps/ the test suite needed half an hour to complete 😞

UniqMartin · 2016-01-28T12:56:04Z

👍 I'm really happy with the solution we arrived at, even if the path wasn't the shortest and most straightforward. The coverage drop is not great, but oh well … it can only get better. 😺

bfontaine · 2016-01-28T13:01:51Z

@MikeMcQuaid @xu-cheng I’ll merge it if you’re happy with the current solution.

MikeMcQuaid · 2016-01-28T13:04:20Z

Library/Homebrew/test/Gemfile

+# See https://github.com/Homebrew/homebrew/pull/48250#issuecomment-173171990
+gem "simplecov", "0.11.1",
+  :git => "https://github.com/Homebrew/simplecov.git",
+  :ref => "a010c8e92c2ea4e2c51f81b52c63b5f03926ccf8",


Cab we point this to a branch instead and link directly to the PRs? Thanks!

MikeMcQuaid · 2016-01-28T13:05:16Z

Couple of final comments then: 👍

MikeMcQuaid · 2016-01-28T13:37:46Z

Library/Homebrew/test/Gemfile

-gem "coveralls", "0.8.9", :require => false
+# This is a patched version of the v0.11.1. Switch back to the stable version
+# when #436 and #437 have been merged upstream.
+# https://github.com/colszowka/simplecov/pull/436


Mind accepting your own PR for that one? It has conflicts and your patch is simple enough to be a separate PR.

I assume you meant “submitting” 😉
Let’s wait until Monday for the 0.11.2 release, which will hopefully fix both issues.

0.11.2 fixed simplecov-ruby/simplecov#436 but not simplecov-ruby/simplecov#437, for which we need to wait for 0.11.3. I updated our fork as well as this PR to use a patched 0.11.2 and I’m writing tests in the meantime to reduce the coverage drop.

MikeMcQuaid · 2016-02-03T20:45:37Z

Looks good 👍

UniqMartin · 2016-02-03T21:53:17Z

👍 Should we merge this and iterate on this later, once upstream resolves the remaining issue that currently affects us?

MikeMcQuaid · 2016-02-03T22:10:57Z

Should we merge this and iterate on this later, once upstream resolves the remaining issue that currently affects us?

👍 from me

We must bump coveralls otherwise we get a conflicting dependency: we use a patched simplecov v0.11.1 but coveralls 0.8.9 needs simplecov 0.10.x.

Closes Homebrew#48250. Signed-off-by: Baptiste Fontaine <batifon@yahoo.fr>

bfontaine added the in progress label Jan 19, 2016

MikeMcQuaid reviewed Jan 19, 2016
View reviewed changes

xu-cheng reviewed Jan 19, 2016
View reviewed changes

bfontaine removed the in progress label Jan 19, 2016

UniqMartin reviewed Jan 20, 2016
View reviewed changes

bfontaine mentioned this pull request Jan 20, 2016

More tests #48281

Closed

bfontaine mentioned this pull request Jan 27, 2016

Fix unloaded files merge simplecov-ruby/simplecov#436

Closed

bfontaine added in progress and removed in progress labels Jan 27, 2016

UniqMartin reviewed Jan 28, 2016
View reviewed changes

MikeMcQuaid reviewed Jan 28, 2016
View reviewed changes

bfontaine added the in progress label Feb 3, 2016

bfontaine added 6 commits February 5, 2016 22:40

formula_assertions: ensure the right Test::Unit is required

17fcb46

tests: don't use Coveralls outside of CI

bf5cf6e

tests: enforce min. 40% coverage

423de03

tests: use our patched simplecov, bump coveralls

e4f2314

We must bump coveralls otherwise we get a conflicting dependency: we use a patched simplecov v0.11.1 but coveralls 0.8.9 needs simplecov 0.10.x.

tests: track all files

feafcbf

tests: fix simplecov handling in integration tests

a4300ec

bfontaine closed this in 1c0ee4b Feb 6, 2016

bfontaine deleted the coverage-fix branch February 6, 2016 13:22

flier pushed a commit to flier/homebrew that referenced this pull request Feb 17, 2016

tests: fix simplecov handling in integration tests

1accd37

Closes Homebrew#48250. Signed-off-by: Baptiste Fontaine <batifon@yahoo.fr>

Homebrew locked and limited conversation to collaborators Jul 10, 2016

tests: Ensure all core files are required #48250

tests: Ensure all core files are required #48250

Conversation

bfontaine commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfontaine commented Jan 19, 2016

UniqMartin commented Jan 19, 2016

bfontaine commented Jan 19, 2016

MikeMcQuaid commented Jan 19, 2016

UniqMartin commented Jan 19, 2016

UniqMartin commented Jan 19, 2016

bfontaine commented Jan 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfontaine commented Jan 20, 2016

xu-cheng commented Jan 20, 2016

UniqMartin commented Jan 20, 2016

MikeMcQuaid commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xu-cheng commented Jan 20, 2016

bfontaine commented Jan 20, 2016

bfontaine commented Jan 20, 2016

bfontaine commented Jan 20, 2016

UniqMartin commented Jan 20, 2016

xu-cheng commented Jan 20, 2016

UniqMartin commented Jan 20, 2016

UniqMartin commented Jan 21, 2016

MikeMcQuaid commented Jan 25, 2016

UniqMartin commented Jan 26, 2016

MikeMcQuaid commented Jan 26, 2016

bfontaine commented Jan 26, 2016

UniqMartin commented Jan 26, 2016

bfontaine commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UniqMartin commented Jan 28, 2016

bfontaine commented Jan 28, 2016

UniqMartin commented Jan 28, 2016

bfontaine commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Jan 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeMcQuaid commented Feb 3, 2016

UniqMartin commented Feb 3, 2016

MikeMcQuaid commented Feb 3, 2016