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

Allow multiple output 'type's for code coverage #193

Closed

Conversation

Iristyle
Copy link
Contributor

The coverage should be reworked to treat type as an array of report output formats...

So that at this point in the code, multiple output formats can be written.
https://github.com/vojtajina/testacular/blob/master/lib/reporters/Coverage.js#L96

I haven't looked too closely at how reports are generated, but as I understand it, all the collection has completed, and reports are simply the way to format the output. I also verified that Istanbul doesn't allow arrays for type, just a string:
https://github.com/gotwarlost/istanbul/blob/master/lib/util/factory.js#L41

Should be a straightforward change to check the param type and either generate one or many reports I think.

/cc @taichi

@taichi
Copy link
Member

taichi commented Nov 28, 2012

istanbul supports multiple output. look like this.
So does not mean that you can make any combination of reports.
You are better to request its function might be better to istanbul.

@Iristyle
Copy link
Contributor Author

Thanks ... hadn't seen that.

However, not sure I like this approach because the combinations would all have to be well-known in advance. I'd rather see arbitrary combinations of reporters passed in.

In other words, this original block of code...

var reporter = istanbul.Report.create(type, options);
try {
  reporter.writeReport(collector, true);
} catch (e) {
  log.error(e);
}

Would then become something like this (assuming type is now an Array and not a string):

type.forEach(function(t) {
  var reporter = istanbul.Report.create(t, options);
  try {
    reporter.writeReport(collector, true);
  } catch (e) {
    log.error(e);
  }  
});

Which seems to effectively do the same thing as the Istanbul code you linked. I'm not sure if the file paths would collide in any way though. I may test this a little later and find out.

- This allows Coberture and HTML reports to be generated at the same
time.
- Should allow any future combinations of reporters to peacefully
co-exist.
- Only allow the file option to work with text and text-summary
- Updated docs
@dignifiedquire
Copy link
Member

@Iristyle Looking good. Two questions,

  • Could you add an e2e test with multiple reporters and
  • have you tested it on a project?

@Iristyle
Copy link
Contributor Author

Yeah, was short on time for the E2E test -- cutting corners ;0

I did test it locally with

        coverageReporter: {
          type: ['html', 'cobertura', 'text'],
          dir: 'coverages/',
          file: 'coverage.txt'
        },
± dir .\coverages -Recurse


    Directory: C:\Source\Osiris\coverages


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
d----        11/28/2012   3:17 PM            PhantomJS 1.7 (Windows)
-a---        11/28/2012   3:17 PM    1827002 coverage-PhantomJS 1.7
                                             (Windows)-20121128_151730.json


    Directory: C:\Source\Osiris\coverages\PhantomJS 1.7 (Windows)


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
d----        11/28/2012   3:17 PM            generated
-a---        11/28/2012   3:17 PM     706245 cobertura-coverage.xml
-a---        11/28/2012   3:17 PM        528 coverage.txt
-a---        11/28/2012   3:17 PM      12129 index.html
-a---        11/28/2012   3:17 PM        676 prettify.css
-a---        11/28/2012   3:17 PM      17569 prettify.js


    Directory: C:\Source\Osiris\coverages\PhantomJS 1.7 (Windows)\generated


Mode                LastWriteTime     Length Name
----                -------------     ------ ----
-a---        11/28/2012   3:17 PM    3443165 app.js.html
-a---        11/28/2012   3:17 PM      12175 index.html

@taichi
Copy link
Member

taichi commented Nov 29, 2012

It looks nice.
I think that String and Array should be set to type is a better.
look like below.

var type = _.flatten([config.type]);

@ghost ghost assigned vojtajina Dec 1, 2012
@Iristyle
Copy link
Contributor Author

Iristyle commented Jan 4, 2013

I'll try and get the tests written to finally finish off this PR today...

@dignifiedquire
Copy link
Member

@Iristyle You have long days ;)

@Iristyle
Copy link
Contributor Author

Hah -- tell me about it! It's like a time warp!

I've been super busy at the day job, hooking up Capistrano style deployments to EC2 from within Hubot (including tracking historical data in it's brain AND logging them to s3). I get to wear the ops guy, build engineer, qa nerd, and "architect" hats, so there's no shortage of stuff to do.

Very soon I'll get a chance to revisit it... I need it for our Janky initiated builds.

@Iristyle
Copy link
Contributor Author

But I think it's about the same as your "tomorrow" 😸

isaacs/node-glob#49

@dignifiedquire
Copy link
Member

Yeah cough I actually did some investigation there but there where so many problems on windows that I never got to finish it. Thanks for the reminder though need to at least let someone know what I found out ...

@dignifiedquire
Copy link
Member

Oh yes finding free time can be very tricky you don't need to tell me that. Between learning for exams, working, looking for a job and looking after my kid I don't get to do all the fun open source stuff I want to do either.

@vojtajina
Copy link
Contributor

Closing this for now. Please re-open once updated.

@vojtajina vojtajina closed this Feb 3, 2013
@psyked
Copy link
Contributor

psyked commented Mar 15, 2013

This would be an awesome feature for lazy developers (like me) who want one configuration file that can be run locally and on a remote server. In my case, I want both "lcov" html output and "cobertura" output. I guess I can keep one locally and overwrite the other on the command line, somehow?

+1 for re-opening this ;)

@makeusabrew
Copy link
Contributor

Also interested in re-opening this as this doesn't appear to be something which can be overridden on the command line, thus necessitating two entirely separate config files in development (where typically html would be the desired format) and CI (where cobertura is preferable) - unless I'm (quite possibly!) missing something :)

@vojtajina
Copy link
Contributor

In the meantime, you can workaround it by doing something like this (in your karma.conf.js):

if (process.env.TRAVIS) {
  coverageReporter = {
    type : 'cobertura'
  };
} else {
  coverageReporter = {
    type : 'html'
  };
}

Btw, I think it's better to not use coverage during development (only on CI), as it instruments all your code, which 1/ makes it little bit slower 2/ makes it harder to debug

@psyked
Copy link
Contributor

psyked commented Apr 7, 2013

@vojtajina Interesting approach - I didn't realise it was possible to do that. Do you know what detection snippet we'd need to use to do with for a Jenkins CI server?

@vojtajina
Copy link
Contributor

Anything that is different on Jenkins than on other environments... eg process.env.JENKINS_URL

@patrickarlt
Copy link

+1 on re-opening this

I like getting coverage reports in the console with the text options as I write tests and the html options so I can dive in and see exactly what I am covering.

@jperl
Copy link

jperl commented Oct 14, 2013

+1

1 similar comment
@bensquire
Copy link

+1

@vojtajina
Copy link
Contributor

I believe this is already implemented, you can do:

coverageReporter: {
  reporters: [
    {type: 'html'},
    {type: 'lcov'}
  ]
}

Btw, I don't think using coverage during development is a good idea.

@psyked
Copy link
Contributor

psyked commented Jan 22, 2014

Thanks @vojtajina, I didn't realise that was a possibility. I can get junit, html and cobertura format stats out of a single test run now, which is an awesome thing to have. Makes pretty graphs on our CI server!

There's a slight issue with the multiple coverage reporters, which is already logged here: karma-runner/karma-coverage#52 but that's not a critical issue!

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

Successfully merging this pull request may close these issues.

9 participants