Add support for simplecov's new expected JSON output #441

f-moya · 2020-09-14T16:09:32Z

Following #413, this PR attempts to add support to simplecov 0.20.0 +. As discussed in the issue shared before, the main idea is that simplecov will natively incorporate a JSON formatter, which we will be triggered by default when running simplecov within an environment that has the CC_TEST_REPORTER_ID variable present.

Accordingly, the simplecov formatter will now try to read coverage information from the output of the new simplecov's JSON formatter. This new report will contain more information reporting as "ignored" those lines marked with :nocov:.

The current implementation pretends to maintain support for previous versions, therefore there are two different paths for looking up the json report, the old .resultset.json and the new coverage.json, a priority in order has been given to coverage.json. When a path is given, like when using test-reporter format-coverate -t simplecov -p /some-path the formatter will try to read the indicated information as if was outputted by the new JSON formatter, in case of that not being the case it will fallback to reading as if it has the same format as .resultset.json. The idea is that no one gets affected by this changes.

As a final note, Until simplecov-ruby/simplecov#923 get's merged or a variation of it, It wouldn't make much sense to merge this work, though in theory it wouldn't cause problems to anyone.

Addresses:

Bonus track

This pull request includes a minimal change on the circleci configuration, was necessary to get CI passing. It just bumps up the XCODE version of the macos engines.

CLAassistant · 2020-09-14T16:09:38Z

All committers have signed the CLA.

noelia-lencina

Nice work! Tests look great 👏 I left a really small comment, feel free to ignore it if you feel it's not worth it.

noelia-lencina · 2020-12-01T13:26:06Z

formatters/simplecov/simplecov.go

+	if err != nil {
+		return legacyFormat(r, rep)
+	} else {
+		logrus.Debugf("Analyzing simplecov json output from latest format %s", r.Path)


nit: would it be possible to move this logic into its own file similar to legacyFormat to keep the same structure?

I liked the suggestion thanks, addressed. Just couldn't find a more clever name to the new formatter than jsonFormatter although it makes sense since it has been designed to read output from the SimpleCov's JsonFormatter.

f-moya · 2020-12-06T23:44:21Z

@filipesperandio can I get your review here please ? I'm posting here since I cannot add you as a reviewer, I think it's due to the repo's permissions settings

filipesperandio · 2020-12-07T20:28:35Z

@fede-moya: You and @noelia-lencina should have write access to the repo now. 🤦

filipesperandio · 2020-12-07T20:30:46Z

formatters/simplecov/simplecov.go

-var searchPaths = []string{"coverage/.resultset.json"}
+var simpleCovJsonFormatterPath, legacyPath string = "coverage/coverage.json", "coverage/.resultset.json"
+
+var searchPaths = []string{simpleCovJsonFormatterPath, legacyPath}


Can this be inlined? Doesn't look like we use those vars anywhere else.

Done 👍🏼

filipesperandio · 2020-12-07T20:32:15Z

formatters/simplecov/simplecov.go

-	if err != nil {
-		return rep, errors.WithStack(errors.Errorf("could not open coverage file %s", r.Path))
-	}
+	rep, err = jsonFormat(r,rep)


I'm confused. Does this override what line 34 does?

Do you mind taking a new look ? I don't think I'm following the the question sorry

I might have something confused in my head, not seeing a problem here anymore 🤦

filipesperandio · 2020-12-07T20:35:35Z

formatters/simplecov/legacy_formatter.go

+	Coverage map[string]formatters.Coverage `json:"coverage"`
+}
+
+func legacyFormat(r Formatter, rep formatters.Report) (formatters.Report, error) {


Seems like we always return rep, can we avoid that and only return error?
And the caller deal with the return it needs to pass? Like return rep, legacyFormat(r, rep)?

Might not be a go thing, though... let me know.

I liked your suggestion so I tested it out, but for some unknown reason to me tests just started to fail without a clear explanation. Check dacd033 , then I reverted the commit and tests passed again. So ...

filipesperandio · 2020-12-07T20:39:48Z

formatters/simplecov/simplecov_test.go

+	rep, err := f.Format()
+	r.NoError(err)
+
+	r.Len(rep.SourceFiles, 7)


The single char vars here are making my eye itchy 🤨
Can we make the names a bit more descriptive? Like, 4 lines down and I don't recall what r is anymore 😬

hahah, yeah I know sorry, I kinda follow the pattern that was already in place, but I have just update all the tests to use better namings.

f-moya · 2020-12-13T22:00:28Z

Hey @filipesperandio just addressed your comments, thanks for your, as usual, detailed review. I'm just pinging you again in this comment to make some noise on your notifications so it became the first thing you see on Monday and luckily if you have further comments we can address them tomorrow 💪🏼

filipesperandio · 2020-12-14T14:41:57Z

Worked. 😂

filipesperandio · 2020-12-14T14:43:24Z

formatters/simplecov/simplecov.go

-	if err != nil {
-		return rep, errors.WithStack(errors.Errorf("could not open coverage file %s", r.Path))
-	}
+	rep, err = jsonFormat(r,rep)


I might have something confused in my head, not seeing a problem here anymore 🤦

filipesperandio · 2020-12-14T14:44:25Z

formatters/simplecov/simplecov_test.go

@@ -18,21 +18,21 @@ func Test_ParseLegacy(t *testing.T) {
 		return s, nil
 	}

-	r := require.New(t)
+	assert := require.New(t)


Nice! thanks!

filipesperandio · 2020-12-14T14:46:18Z

.circleci/config.yml

@@ -24,7 +24,7 @@ jobs:

  test_macos:
    macos:
-      xcode: "9.0"
+      xcode: "11.3.0"


Any chance we can do this in a separate PR? Feel free to tag me.
Just want to isolate infra/tooling changes from other things, makes it easier to revert in case we need to.

yeap, I think that makes sense. Will do that now

This reverts commit dacd033.

codeclimate-hermes assigned filipesperandio Sep 14, 2020

noelia-lencina approved these changes Dec 1, 2020

View reviewed changes

efueger requested a review from filipesperandio December 7, 2020 19:44

filipesperandio reviewed Dec 7, 2020

View reviewed changes

filipesperandio approved these changes Dec 14, 2020

View reviewed changes

f-moya and others added 11 commits December 14, 2020 15:15

Add support for simplecov's new expected output

4794135

Rename existent tests

a6088be

Refactor to include simplecov-latest in simplecov.go

f04e20e

Add tests

f6e192a

Format code

f88e527

Address Noe's refactoring feedback

869e7fb

Avoid using a variable for each search path

c0480e4

Use only one return statement for returning the analysis result

c9b63d2

Avoid unnecesary return of rep on each dedicated formatter

c55c600

Revert "Avoid unnecesary return of rep on each dedicated formatter"

3267fbc

This reverts commit dacd033.

Use better var names for testing

6a7e32b

f-moya merged commit d3d488b into codeclimate:master Dec 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for simplecov's new expected JSON output #441

Add support for simplecov's new expected JSON output #441

f-moya commented Sep 14, 2020 •

edited

Loading

CLAassistant commented Sep 14, 2020 •

edited

Loading

noelia-lencina left a comment

noelia-lencina Dec 1, 2020

f-moya Dec 6, 2020 •

edited

Loading

f-moya commented Dec 6, 2020

filipesperandio commented Dec 7, 2020

filipesperandio Dec 7, 2020

f-moya Dec 13, 2020

filipesperandio Dec 7, 2020

f-moya Dec 13, 2020

filipesperandio Dec 14, 2020

filipesperandio Dec 7, 2020

filipesperandio Dec 7, 2020

f-moya Dec 13, 2020

filipesperandio Dec 7, 2020

f-moya Dec 13, 2020

f-moya commented Dec 13, 2020

filipesperandio commented Dec 14, 2020

filipesperandio Dec 14, 2020

filipesperandio Dec 14, 2020

filipesperandio Dec 14, 2020

f-moya Dec 14, 2020

Add support for simplecov's new expected JSON output #441

Add support for simplecov's new expected JSON output #441

Conversation

f-moya commented Sep 14, 2020 • edited Loading

Addresses:

Bonus track

CLAassistant commented Sep 14, 2020 • edited Loading

noelia-lencina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-moya Dec 6, 2020 • edited Loading

Choose a reason for hiding this comment

f-moya commented Dec 6, 2020

filipesperandio commented Dec 7, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f-moya commented Dec 13, 2020

filipesperandio commented Dec 14, 2020

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

f-moya commented Sep 14, 2020 •

edited

Loading

CLAassistant commented Sep 14, 2020 •

edited

Loading

f-moya Dec 6, 2020 •

edited

Loading