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

fix: Don't use OS specific browser name in directory name. Fix #61 #62

Closed
wants to merge 1 commit into from

Conversation

SimplGy
Copy link

@SimplGy SimplGy commented Feb 20, 2014

Notes in #61. Looking for a way to keep the folder name the same no matter what environment you're on.

If you want to use the coverage output in any kind of reliable way, in needs to be in a fixed location. This is one way to do it, but I don't love the solution :)

@vojtajina
Copy link
Contributor

I'm not that excited about this solution either ;-)

@vojtajina
Copy link
Contributor

Anybody has any ideas how to solve this better?

@vojtajina
Copy link
Contributor

Maybe we could allow "placeholders" in the path config, something like "./coverage/%browser.name% (%browser_platform%)/"

@maethorechannen
Copy link

How about a simple flag to turn off/on adding the browser name and platform? I don't really need the browser name/platform on the CI server at all.

To be honest, I wasn't expecting it to be added in the first place - I was expecting the output to be in the directory I configured in coverageReporter. The added browser name was a surprise.

@blittle
Copy link

blittle commented Mar 26, 2014

I am confused as to why it requires the browser folder in the first place, especially because it is inconsistent with the output of other reporters (ie the junitReporter)

@SimplGy
Copy link
Author

SimplGy commented Mar 31, 2014

I think the browser name is surprising, too. It seems to be expecting a run of coverage against each browser -- would that ever be different in different environments? I don't know. It's not a feature I need, I only use coverage on CI.

@codedogs
Copy link

I see value in the existing directory structure naming when using many browsers with Karma. For the one report that I want to publish, it's easy to add an action after karma runs to copy the PhantomJS report to a directory without the browser name.

@SimplGy
Copy link
Author

SimplGy commented Apr 15, 2014

@codedogs: two follow ups:1- how would you automate a copy if the directory name can change? Does karma expose a variable or something?

2- why would you run coverage on multiple browsers?

On Mon, Apr 14, 2014 at 2:37 PM, codedogs notifications@github.com
wrote:

I see value in the existing directory structure naming when using many browsers with Karma. For the one report that I want to publish, it's easy to add an action after karma runs to copy the PhantomJS report to a directory without the browser name.

Reply to this email directly or view it on GitHub:
#62 (comment)

@blittle
Copy link

blittle commented Apr 15, 2014

Totally agree with @SimpleAsCouldBe It doesn't make as much sense to do code coverage in separate browsers as it does to get general test output. It is weird that it is inconsistent with the junit test output path structure (and in that case I care more about running in separate browsers).

@codedogs
Copy link

If one has javascript that uses feature detection or browser sniffing, javascript execution will be execute browser-specific branches in different browsers. For example, if you're using a shim for IE8, then testing code coverage in IE8 would be different than in PhantomJS and other browsers.

@codedogs
Copy link

Regarding automating the directory copy to a new name, include this in your grunt file to run clean before karma and copy after karma (also clean all coverage reports if they can exist before running):

        clean: {
            karma: ['target/test/coverage/PhantomJS/']
        },
        copy: {
            karma : {
                nonull: true,
                mode: true,
                expand : true,
                flatten : false,
                src : ['PhantomJS*/**'],
                dest : 'target/test/coverage/PhantomJS/',
                cwd : 'target/test/coverage/',
                rename: function(dest, src) {
                    // remove the first directory in src, e.g., PhantomJS 1.9.7 (Mac OS X)/
                    // because we're copying to PhantomJS/
                    return dest + src.split('/').slice(1).join();
                },
                filter : 'isFile'
            }
        }

@SimplGy
Copy link
Author

SimplGy commented Apr 18, 2014

A very good point on browser-specific code branches, @codedogs, and a great, if lengthy, grunt solution for folder name flattening. Thank you.

Still we prolly want an option to turn off the folder names, ya? Most people, I'd venture, aren't interested in browser-routed coverage.

@andersekdahl
Copy link

I'd love a way to generate coverage without ending up with an extra folder. Vojtas suggestion with "./coverage/%browser.name% (%browser_platform%)/" would be great.

@aymericbeaumet
Copy link
Member

Instead of using placeholders (which I find are not readable), we could add a subdirectory (subdir) option. It would define in which subdirectory the coverage should be created. It could take a string or a function:

  • In the case a string is passed, the option would be set directly.
  • In the case a function is passed, the option would be set to the function return value. The function will be called passing the browser name and the platform as arguments.

Do not pass the subdir option (actual behavior)
module.exports = function(config) {
  config.set({
    coverageReporter: {
      dir: 'coverage'
      // Would output the results into: './coverage/BROWSER (PLATFORM)/'
    }
  });
};
Pass the subdir option (as a string)
module.exports = function(config) {
  config.set({
    coverageReporter: {
      dir: 'coverage',
      subdir: '.'
      // Would output the results into: .'/coverage/'
    }
  });
};
Pass the subdir option (as a function)
module.exports = function(config) {
  config.set({
    coverageReporter: {
      dir: 'coverage',
      subdir: function(browser, platform) {
        // normalization process to keep a consistent browser name
        return browser.toLowerCase().split(' ')[0];
      }
      // Would output the results into: './coverage/firefox/'
    }
  });
};

@andersekdahl
Copy link

@aymericbeaumet, I like it, but wouldn't it be simpler to skip subdir and make it possible to let dir be a function that can return coverage/firefox? Would perhaps be a breaking change that the dir property no longer adds the platform to the path, but it wouldn't be a big issue for most people.

@aymericbeaumet
Copy link
Member

As you pointed, I actually made this solution up because modifying the dir options would be a breaking change. This is why I actually only enriched the existing semantic:

  • dir is used to defined the global coverage dir
  • subdir is used to defined how are the browsers subdirs name generated/named

@blittle
Copy link

blittle commented May 21, 2014

@aymericbeaumet I like that solution.

@aymericbeaumet
Copy link
Member

@blittle Well give it a shot if you feel being able to do it 👍

@blittle
Copy link

blittle commented May 21, 2014

@aymericbeaumet I'll see if I can fit in some time tonight :)

@codedogs
Copy link

I will try that too. Great solution--thanks!

@lucdubois
Copy link

coverageReporter: {
        type : 'clover',
        dir : 'tmp/coverage/jasmine',
        file: '../clover.xml'
    },

The folder is still created, but the file is in the right directory.

@bilalq
Copy link

bilalq commented Jun 18, 2014

+1 for subdir support. I'm creating copy tasks to get around this right now, which is quite a hassle.

@aymericbeaumet
Copy link
Member

I will dig into it as @blittle and @codedogs seem to have given up.

@bilalq
Copy link

bilalq commented Jun 19, 2014

Cool! And not to dissuade you from doing this, but this issue isn't specific to karma-coverage. The karma-html-reporter package also outputs its test results under a <Browser> (<OS>) directory.

For the benefit of anyone looking for a short term solution, the copy target that @codedogs posted earlier works, but note that the trailing slash in directories names there is not optional, and the call to join should have '/' as an argument.

@daniellmb
Copy link

+1 for the subdir option. Currently I'm able to get around it using a glob pattern but it'd be great to have a clean way to control it.

@jscti
Copy link

jscti commented Jul 17, 2014

+1 for subdir

@stramel
Copy link
Contributor

stramel commented Jul 25, 2014

+1 for subdir

@daniellmb How are you using a glob pattern to get around it?

aymericbeaumet added a commit that referenced this pull request Jul 25, 2014
We discussed in this [issue] about the problem of not being able to choose the
complete output directory.

This commit addresses this issue and add the new `subdir` option which allows to
specify the full output directory of each coverage report.

[issue]: #62
aymericbeaumet added a commit that referenced this pull request Jul 25, 2014
We discussed in this [issue] about the problem of not being able to choose the
complete output directory.

This commit addresses this issue and add the new `subdir` option which allows to
specify the full output directory of each coverage report.

[issue]: #62
@aymericbeaumet
Copy link
Member

@stramel I think he's using a glob pattern in order to match whatever the browser name (browser + version + OS) is. This is useful when dealing with Travis.

Btw, I implemented what I described above (the subdir option). Can you guys give it a try? I'm waiting for your feedbacks here.

@daniellmb
Copy link

@stramel
Copy link
Contributor

stramel commented Jul 25, 2014

@daniellmb @aymericbeaumet Thank you for the explanation and link! It wasn't exactly what I was looking for but I think the subdir option will handle what I am needing.

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

Successfully merging this pull request may close these issues.