Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle multi-command resultsets #163

Merged
merged 4 commits into from
Dec 29, 2016
Merged

Handle multi-command resultsets #163

merged 4 commits into from
Dec 29, 2016

Conversation

pbrisbin
Copy link
Contributor

Simplecov supports merging coverage from multiple test suites by writing them
at different command names in the resultset file, then merging them when
building the report.

We were getting this for free pre-1.0 because the resultsets would be merged
before being passed to the custom formatter. Post-1.0 we were (mistakenly)
assuming it'd just be one command name in the JSON file, hence we were only
reading:

SimpleCov::Result.new(results.values.fetch(0).fetch("coverage"))

discarding all but the first command-name.

This is both broken and more work than we need to do. The file has a
well-defined schema, and we can rely more on Simplcov for parsing it.

Something like:

SimpleCov::Result.from_hash(results.first)

would've been functionally equivalent, and shows more clearly the bug. What we
really want is:

results.map do |k, v|
  SimpleCov::Result.from_hash(k => v)
end

Then with an array of results, we can in-line some of Simplecov::ResultMerger
to merge them together into a single Simplecov::Result that we'll format for
upload. Unfortunately, we do have to in-line the method we need because this
object has multiple responsibilities (yay!), including caching and re-writing
files on disk, which we don't want.

Fixes #7

/cc @codeclimate/review

If someone's around and wants to merge / release that'd be great. If not, I'll
pick it up when I'm back next week.

- Use a support helper for managing a project fixture
- In-line the lets
Simplecov supports merging coverage from multiple test suites by writing them at
different command names in the resultset file, then merging them when building
the report.

We were getting this for free pre-1.0 because the resultsets would be merged
before being passed to the custom formatter. Post-1.0 we were (mistakenly)
assuming it'd just be one command name in the JSON file, hence we were only
reading:

    SimpleCov::Result.new(results.values.fetch(0).fetch("coverage"))

discarding all but the first command-name.

This is both broken and more work than we need to do. The file has a
well-defined schema, and we can rely more on Simplcov for parsing it.

Something like:

    SimpleCov::Result.from_hash(results.first)

would've been functionally equivalent, and shows more clearly the bug. What we
really want is:

    results.map do |k, v|
      SimpleCov::Result.from_hash(k => v)
    end

Then with an array of results, we can in-line some of Simplecov::ResultMerger to
merge them together into a single Simplecov::Result that we'll format for
upload. Unfortunately, we do have to in-line the method we need because this
object has multiple responsibilities (yay!), including caching and re-writing
files on disk, which we _don't_ want.

Fixes #7
@maxjacobson
Copy link
Contributor

Nice! Going to review more closely a littler later and may ship it this week. Looks like this build is failing for reasons unrelated to your change, which should be fixed here: #164

Copy link
Contributor

@maxjacobson maxjacobson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM, and I did a small amount of QA on a repo with only one command in the resultset. The tests look good, though. Planning to ship tomorrow.

FileUtils.rm_rf("#{@old_pwd}/spec/fixtures/fake_project")
FileUtils.cd(@old_pwd)
end
it "addresses Issue #7" do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this spec description :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants