Insufficient test coverage to ensure that compiler releases work with Angular

[alexeagle]

Angular is selling our users on Closure Compiler - as of Angular 4 we provide some support at http://github.com/angular/closure-demo
It's also a key part of our "develop like google does" story: http://g.co/ng/abc

However, we frequently find that closure releases are broken. Currently 20170409 is the latest release that works with Angular - however it won't work with Angular 5 because we need some recent fixes to node module resolution.

We should ensure that a green build of closure will work with Angular.

we should be able to copy what closure-library does to run their end-to-end tests externally, and use that to run the closure compiler IntegrationTest (currently it only runs in google-internal)

/cc @gregmagolan @MatrixFrog

[alexeagle]

@MatrixFrog is this the place you suggest we could add missing tests?

closure-compiler/test/com/google/javascript/jscomp/IntegrationTest.java

Line 48 in 35d9e29

This fixture seems insufficiently powerful to reproduce issues that depend on Angular artifacts, like https://github.com/angular/closure-demo/blob/master/closure.conf does

At a minimum, given the close relationship of Angular team and closure compiler, can we provide a manual test case to be run before closure releases to check that it's not broken?

[MatrixFrog]

Our release process is pretty involved already. I'd prefer an automated test, even if it only runs inside Google.

[brad4d]

@alexeagle could you add more information here about the kind of breakages you're seeing and what tests would catch them?

[gregmagolan]

@MatrixFrog I put together the closure compiler build with the fixes required to get https://github.com/angular/closure-demo building with the latest closure compiler code.

Fix fixes are here:
gregmagolan@5f21b3b

I'm new to the code base but the breakages as far as I can tell are:

1. When parsePotentialModules() is run in Compiler.java, the JsFileParser it uses ends up using the ModuleLoader.EMPTY module loader with BROWSER resolution mode and the compiler doesn't find the angular packages during that stage. Adding ModuleLoader moduleLoader = compiler.getModuleLoader(); and .setModuleLoader(moduleLoader) on the JsFileParser in CompilerInput.java sets the correct module loader in JsFileParser during parsePotentialModules(). It is set to the requested module loader: ether NodeModuleLoader or BrowserModuleLoader depending on the closure config. The closure-demo uses the NodeModuleLoader.

2. Some of the imports in the angular/closure-demo are ambiguous (not absolute or relative paths) but are not resolved with resolveJsModuleFromRegistry so the other fix allows those to work by trying resolveJsModuleNodeFileOrDirectory and then resolveJsModuleFromRegistry if the first doesn't resolve.

[gregmagolan]

There are some other issues that come up when I tried pulling in PR #2641. Not sure if the closure team is pulling that in to the next release.

Without my patches it fails silently and generates a tiny bundle. With my patches it still fails silently and generates a bundle with require() calls in what looks like the RxJS code and it re-orders the Angular packages putting them in an order that fails at runtime.

[MatrixFrog]

Yes, I'm working on getting the google repo into a state where 2641 can be submitted. Hopefully will be done by the end of this week.

[ChadKillingsworth]

@gregmagolan I have a large set of changes queued up for CommonJS processing. However, they all depend on #2641 so you won't see any movement there until it lands.

[gregmagolan]

@ChadKillingsworth That's good to hear. Are they on your fork? I can build them locally and try the changes out with https://github.com/angular/closure-demo

[ChadKillingsworth]

@gregmagolan You can test with this JAR: https://github.com/ChadKillingsworth/closure-compiler-npm/blob/webpack/compiler.jar

But no, the code is still being cleaned up and targeted for separate PRs.

[alexeagle]

The ideal test, in my mind, is to reproduce the angular/closure-demo CI: an integration test against the closure.jar that fetches a fixed Angular release, builds an Angular app with our recipe, and runs a protractor test that the app still works. That gives the most realistic coverage of bugs, and is obvious what it tests and how it reproduces a user's reported problem - but I'd also understand if that fixure sounds too large/slow for you.

The next closest thing is to take that example apart one step: figure out what closure compiler command line we call, what the input .JS files look like in this Angular app, and hard-code them as a golden test. Then we have to accept a new golden output file from time-to-time, which we would do by confirming the new output bundle still works.

We could take it apart even further, but then I'm not sure we know what the right assertions are. The app breaks in new and interesting ways every time.

[MatrixFrog]

Can you sync the closure-demo into google3? I'd prefer a test that fails when we try to submit a compiler change, to one that fails when we do a release, which may be weeks after the actual breaking change went in.

[alexeagle]

I don't think it's possible, this relies on different output from ts_library (using ES Modules) and different filesystem layout (node module resolution)

…

On Wed, Sep 27, 2017 at 11:50 AM Tyler Breisacher ***@***.***> wrote: Can you sync the closure-demo into google3? I'd prefer a test that fails when we try to submit a compiler change, to one that fails when we do a release, which may be weeks after the actual breaking change went in. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2664 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC5I4S5qqDgE275sXtqR85S_XzopaCsks5smpjzgaJpZM4PjI6D> .

[brad4d]

We have an internal test that confirms that a change won't break the opensource maven build.
Could someone familiar with the closure-demo code write an internal test that does this?

1. get the opensource equivalent of our internal code and build it (closure and closure-compiler), using our opensource maven build test as a model
2. use git clone to pull down the current closure-demo code
3. test the closure-demo code using the version of closure & closure-compiler created in step 1

[gregmagolan]

@ChadKillingsworth I opened up two PR against the walk-dep-tree branch on your fork.

ChadKillingsworth#1
ChadKillingsworth#2

Can you take a look and let me know what you think?

[alexeagle]

FYI we just fixed Angular to reference a released closure compiler rather than a fork:
angular/angular@161f88f
so I think someone could pick up this issue.
Sadly almost everyone working on this is external so we either need to move the testing to external repo, or find a Googler who has time/interest.

[ChadKillingsworth]

@alexeagle Once all my webpack PRs land, we are considering doing this in the https://github.com/webpack-contrib/closure-webpack-plugin repo - possibly using the angular demo as the test case.

CC @d3viant0ne

[joshwiens]

@alexeagle - I've got a CI setup that can accommodate integration suites that are framework specific in the plugin.

You can find me in the Webpack slack or the flex-layout room in the Angular slack if you want / need to discuss it further.