Replace jscoverage with karma-coverage

[mramato]

1. Replace our ancient, windows-only jscoverage implementation with karma-coverage, which works on all platforms and does not require a separate instrumentation step. You now run coverage by simply typing npm run coverage. This also provides improved results, such as branch coverage, which the old system did not.
2. Updated Testing Guide
3. Remove old instrument link from index.html
4. Remove jscoverage and related code.

Current stats
90.38% Statements 81603/90285
81.91% Branches 27629/33729
90.95% Functions 9010/9906
90.38% Lines 80611/89193

these numbers were lower than I expected, but Source/Workers is uncovered by unit tests (at least directly), we should probably add some tests for them and it would get back above 93% pretty easily. I didn't want to lie to ourselves by simple excluding them.

For anyone wondering why we stayed on the old system for so long, I actually made this change ~3 years ago but karma-coverage fixed a showstopping bug ~8 days ago and that finally allowed us to use it.

[cesium-concierge]

Thanks for the pull request @mramato!

• ✔️ Signed CLA found.
• ❔ CHANGES.md was not updated.
  • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
• ❔ Unit tests were not updated.
  • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.
• Works (or fails gracefully) in IE11.

[pjcozzi]

😍. What is involved to have this on CI?

[mramato]

What is involved to have this on CI?

Pretty trivial, basically involves running the unit tests a second time under coverage and then our existing deployment script should just copy the results up to the server. Probably 30 minutes worth of work total to get the details right. We may even be able to just replace the first unit test run with coverage and still fail the build if any tests fail.

The only gotcha would be that coverage results would probably be slightly decreased because we are using the webglstub instead of running with full webgl. Perhaps that something we could fix by improving the tests themselves, but we would have to look into it.

A local run with webglstub set shows:

86.84% Statements 78401/90285
77.62% Branches 26179/33729
85.33% Functions 8453/9906
86.84% Lines 77452/89193

So we lose ~4% initially do to the stub.

[mramato]

I forgot to add, during development this now lets you run coverage on a single test or suite, which is incredibly useful when adding a new feature or class to ensure that the code is being intentionally covered by your new tests and not accidentally covered (and untested) by existing tests. Simply use fdefineSuite/fdescribe/fit just like you would if running a single test.

[pjcozzi]

Would suggest to just run with it (without updating WebGL tests for now) and/or just submit an issue.

[mramato]

Sounds good. I'll probably do it in a separate PR as soon as I have time (and to make it easily reviewable), no need to hold this up assuming others are good with it.

[mramato]

Since no one looked at this yet, I just added CI support directly to this PR.

1. Add options from the test task to the coverage task
2. Run coverage instead of test on CI (since covereage includes text)
3. Add link to coverage results in index.html and also as part of the the GitHub deploy information. You can see this below where you would normally have the build/deployment links, there is now coverage results.

[mramato]

@OmarShehata maybe you can take a look at this? The change itself is pretty straightforward.

[OmarShehata]

If you run npm run coverage first, then npm run coverage --browsers Firefox,Chrome this is going to open over a dozen tabs, most of which don't work, because readdirSync doesn't just return directories, it returns files too.

Consider changing this to something like:

var pathNames = fs.readdirSync('Build/Coverage');
    pathNames.forEach(function(pathName) {
        if (fs.statSync(path.join('Build/Coverage', pathName)).isDirectory()) {
            open(path.join('Build/Coverage', pathName, 'index.html'));
        }
    });

[mramato]

Yeah, this was me trying to give us a well-known location for index.html and the deployment scripts. I may just punt and have coverage always be a single browser. It's really uncommon to need to run coverage on multiple browsers at once, and in most cases the results are identical

[OmarShehata]

This is wonderful! We can now see coverage results in the browser for every PR (but with WebGL stubs):

http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/coverage/Build/Coverage/index.html

And it would be easy to compare against master (which should get built in a similar URL right?) It's also pretty easy to generate coverage for just a single class as described above. The updated doc looks good.

Some recommendations/questions:

• Why was npm run clean removed in .travis.yml?
• We no longer need this Instrumented folder in the filesToClean list right? https://github.com/AnalyticalGraphicsInc/cesium/blob/cb496c6271694c4e6a11f446d34d37e45185b6b5/gulpfile.js#L70
• The only thing is the command line options for npm run coverage aren't really documented anywhere (so you'd have to read the code to find out --browsers takes a comma separated list) but maybe that's OK.

[mramato]

• Why was npm run clean removed in .travis.yml?

Because otherwise the coverage results get deleted and wouldn't be deployed. I'm actually the one that added the clean (4 years ago apparently) and I think it was to ensure no extra files from earlier in the process were left around when makeZipFile was run, but I'm pretty confident it's not needed ad the resulting zips are the same, so it's safe to remove (so that coverage gets deployed). Coverage itself is not in the zip, just on AWS.

• We no longer need this Instrumented folder in the filesToClean list right?

No, good catch. There are a few places that need to be updated.

• The only thing is the command line options for npm run coverage aren't really documented anywhere (so you'd have to read the code to find out --browsers takes a comma separated list) but maybe that's OK.

This is true for a lot of our scripts. Could be a good code sprint item to clean things up but it certainly hasn't been an issue so far.

[mramato]

@OmarShehata it's over-engineered, but we now auto-generate an index.html with a list of browsers with coverage results. So it works with multiple browsers without any kind of weird folder behavior.

This is ready.

[mramato]

Bump

[OmarShehata]

I like this - this works pretty well now!