Add updated browser spec runner

[ggetz]

Fixes #10134
Fixes #10200

This returns the browser spec runner. However, instead of copying the standalone runner into a third party folder and making customizations, this is a bit more minimal version which loads files from jasmine-core directly and then configures jasmine as needed through existing API. I ended up not using the jasmine-browser-runner NPM module since it was mostly just a server wrapper around code which exists in the jasmine-core package already, and the server did not allow us to serve files other than JS or CSS which are needed for many of our specs.

• Remove custom cesium css as it no longer applies to the browser runner's page structure (so the progress bar doesn't exist anymore 😢 )
• Removes our custom it, describe, beforeEach, afterEach, etc implementations which checked for a promise and manually called done upon resolution. This is no longer needed with the updated version of jasmine supports returning promises directly.
• Fix a few specs which had deprecation errors, namely those mis-using done
• When running in the browser, I noticed that the custom toThrowDeveloperError macther function was not handling instanceof correctly, so I tweaked the condition
• Modified how we handle categories. With the latest version of jasmine, if a describe block is empty, it throws an exception and fails the suite. So instead of early returns, we now call xdescribe instead. (Also note that the jasmine api does not support a third parameter, so no need to pass the category to the original describe function). I don't believe any singular test or it block had the WebGL category, so I removed the custom it handler for categories.
• Modified how command line spec filter parameters work. They now use the same logic as the ?spec= query in the browser runner, as built-in to the karma-jasmine package already in use. This removes the --excludeName option as there is no counterpart in the browser query
• Jasmine files are copied into the Spec folder during the prepare step which runs after install. Since we move them to the Specs directory, they automatically get deployed along with the rest of that folder and can be accessed via s3, for example http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/jasmine-browser/Specs/SpecRunner.html?category=none
• One thing that I was really confused by what the 'built' query should be doing for the specs. It tests on un-minfied cesium, yes, but I wasn't sure if it should include the tests that check for DeveloperErrors or not. Running combine (no pragmas removed) and combineRelease (debug pragmas removed) build to the same location, so whether or not the debug pragmas are removed is from independent from which set of tests you chose to run. So in the end, running combineRelease and then running the tests in browser with built=true&release=false results in failing specs, but I believe this is consistant with what the behavior was before.
• For consistency it would be great to be able to run built, but not release, test on the console for consistency with the browser, but I'll leave that out of this PR for scoping reasons.

[cesium-concierge]

Thanks for the pull request @ggetz!

• ✔️ 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.

Reviewers, don't forget to make sure that:

• Cesium Viewer works.
• Works in 2D/CV.

[sanjeetsuhag]

@ggetz It's great to have the UI back, but it seems like there's a significant performance regression when it comes to loading the tests. I compared with version 1.87. I ran npm i; npm run build; npm start on both branches tested and ran the Core/AssociateArray spec suite.

	jasmine-browser	1.87
Time to run Core/AssociativeArray	9.568s	1.467s
Webpage
Network Debugger

It seems like the time to load a single JS file has increased significantly. For example, createGlobe.js went from 12 ms to 63 ms. Is this because we are not using jasmine-browser-runner?

[sanjeetsuhag]

I would also not mark #10262 as fixed by this PR, since that is related to the karma CLI test command, which still fails on Safari on this branch.

[ggetz]

@sanjeetsuhag I was importing a few cesium helper function a way that included Cesium.js where I didn't need to. I removed that, which helps performance overall, but this is still a bit slower than 1.87, which I'll investigate further.

[ggetz]

After some investigation, I'm pretty sure this is due to the jasmine and the HtmlReporter itself, and of having a lot of specs. Page load times themselves are comparable between branches, as well as the total time to run all specs. But if you turn off the hideDisabled option in this branch, you can see that even when filter specs, it's iterating through each one and adding symbols to the DOM. The same thing happens when using fit or fdescribe. I'm not sure if its possible to circumvent this behavior entirely, but it's really not great to introduce such a big delay for running a subset of specs.

[ggetz]

@mramato Would you have a bit of time to sanity check the speed of focused specs here? Is it worth rolling back to the older version of jasmine if the new one introduces a such a delay for focused specs?

[mramato]

@ggetz happy to take a look

[mramato]

@ggetz I checked out this branch, did a clean npm install run npm run build and then npm start I clicked on run all tests on http://localhost:8080/ and it just fails to load with the below console errors. What am I doing wrong?

Refused to apply style from 'http://localhost:8080/Specs/jasmine/jasmine.css' because its MIME type ('text/html') is not a supported stylesheet MIME type, and strict MIME checking is enabled.
SpecRunner.html:9          GET http://localhost:8080/Specs/jasmine/jasmine.js net::ERR_ABORTED 404 (Not Found)
SpecRunner.html:10          GET http://localhost:8080/Specs/jasmine/jasmine-html.js net::ERR_ABORTED 404 (Not Found)
SpecRunner.html:11          GET http://localhost:8080/Specs/jasmine/boot0.js 404 (Not Found)
SpecRunner.html:13          GET http://localhost:8080/Specs/jasmine/boot1.js net::ERR_ABORTED 404 (Not Found)
SpecRunner.html:28 [Violation] Avoid using document.write(). https://developers.google.com/web/updates/2016/08/removing-document-write
(anonymous) @ SpecRunner.html:28
spec-main.js:32 Uncaught ReferenceError: jasmine is not defined
    at spec-main.js:32:1
(anonymous) @ spec-main.js:32
ApproximateTerrainHeightsSpec.js:6 Uncaught ReferenceError: describe is not defined
    at ApproximateTerrainHeightsSpec.js:6:1

[ggetz]

@mramato Sorry, run build-specs before npm start to copy the jasmine files over. Updated the PR description.

[ggetz]

@mramato Were you able to get test running?

[mramato]

@ggetz Sorry, once I had issues I wasn't able to get back to it. Why is build-specs needed?

[mramato]

Also, are we talking about focused tests (i.e. fit) or "test I click on in the list". Because focused tests seem plenty fast for me.

[mramato]

I took a look at the "test I click on the list" performance and nothing obvious jumped out at me.

One stab in the dark is that our use of HTTP 1.1 instead of HTTP2 is slowing things down because of the massive number of individual JS files Cesium loads on start.

Actually using the fit to focus seems faster (and honestly that's how I usually develop anyway) so just using that could be a good workaround for now.

Overall, I think we are going about this the wrong way in that we are working from top down instead of bottom up. My approach would have been to fix/improve Cesium's overall build infrastructure first and then update how we handle stuff like tests based on that(because it's going to have to be tweaked again anyway).

[ggetz]

No problem, thanks for taking a look!

build-specs currently copies over the jasmine files from node_modules. I'm not sure that's the final approach, but it does allow the specs to be run in the s3 deployed copy linked from the PR.

I noticed the delay for both focused (fit) specs and "test I click on the list".

Fair point about the the order of approach. I agree that loading approach will likely change after adjusting the build infrastructure.

[mramato]

build-specs currently copies over the jasmine files from node_modules

I assumed that was the case. I would recommend doing it in the prepare task, which runs after npm install and is automatically removed already as part of the release process.

[ggetz]

Since the "test I click on the list" performance likely hinges on changes that will likely come with re-assessing the build infrastructure, let's put a pin in that for now.

@ebogo1 Could you please give this a final review?

[ebogo1]

Looks good! Browser runner works as advertised.

• The WebGL tests link on index.html shows a bunch of yellow asterisks and dots for excluded tests. Not sure if there's a way to change this but I think it's making the page run slower.
• Should the release guide should be updated with a link to the web runner once this is merged?
• Probably not in the scope of this PR, but running tests with WebGL validation isn't part of the release process anymore. Should we open an issue to keep track of that option? Quote from @mramato when we discussed offline a little while back:

We should also understand if anyone ever actually uses that testing option (I don't think they do) and if that's because it's slow or because it's not useful?

We should either remove it completely or figure out why it's so slow based on the feedback from other WebGL devs.

I had a couple other comments and questions -

[ebogo1]

Can includeName also be removed? The changes in karma-main.js no longer do anything with the argument unless I'm missing something. There's a section in the testing README that we can also delete.

[ggetz]

We removed our custom logic in karma-main.js, however we're now hooking into the --grep option which exists as part of karma-jasmine. This option uses the same function under the hood via jasmine-core as the ?spec= query does in the browser runner.

[ebogo1]

Just curious, why was this needed?

[ggetz]

Not strictly needed, but this doesn't require all of Cesium.js until it's needed by the specs. Again, this is something that will likely need to be re-examined as a part of investigating the build process, so I think I'll actually revert this change for the time being, as it doesn't affect performance either way.

[ebogo1]

Also, some sections about the browser runner were deleted from the testing README in #10184, could those be added back?

[ggetz]

The WebGL tests link on index.html shows a bunch of yellow asterisks and dots for excluded tests. Not sure if there's a way to change this but I think it's making the page run slower.

We can't filter by category up front like with we do with the spec name because categories are not a part of the jasmine API, and we don't get access to it until we intercept it function. For me, webgl only tests was still faster than running all the tests by more than a minute. I think any slowness is due to the same one as discussed above for running one test.

Should the release guide should be updated with a link to the web runner once this is merged?

As long as there is a way to run the release tests, I don't think it's critical that its using the browser. However I can certainly update to include it as an alternative.

Probably not in the scope of this PR, but running tests with WebGL validation isn't part of the release process anymore. Should we open an issue to keep track of that option?

While its alluded to in other issues, it didn't have its own. Opened #10309.

[ggetz]

Thanks @ebogo1, updated!

[ebogo1]

Cool, thanks @ggetz - looks good to me. Only last question is whether we want to add the browser runner to the release zip index.html too - that link was also removed in #10184 from index.release.html.

[ggetz]

Based on my suggestion in #3911, that would mean putting the Spec folder back in. So the spec runner would come along in that folder. I plan on opening a subsequent PR with those changes.

[lilleyse]

Thanks @ggetz, much appreciated to have this back!