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

feat(upgrade): provide test helpers for wiring up injectors (WIP) #16848

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
@petebacondarwin
Copy link
Member

petebacondarwin commented May 17, 2017

This PR provides two test helpers for the upgrade/static library, which wire up the Angular and AngularJS injectors without the need for a full bootstrap of a hybrid app.

  • createAngularJSTestingModule() should be used in AngularJS tests
  • createAngularTestingModule() should be used in Angular tests

Check out the spec files for example usage.

TODO: this probably does not support testing upgraded/downgraded components.

@googlebot googlebot added the cla: yes label May 17, 2017

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch from bfe4205 to c73b392 May 17, 2017

@mhevery

This comment has been minimized.

Copy link
Member

mhevery commented May 17, 2017

Looking good! I think we need some more documentation on how to use this. Can this be part of the API docs?

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented May 17, 2017

I'll add docs but wanted feedback on whether this is the right API first.

*/

import {TestBed} from '@angular/core/testing';
import * as angular from '@angular/upgrade/src/common/angular1';

This comment has been minimized.

@juliemr

juliemr May 17, 2017

Member

Is this line required?

This comment has been minimized.

@petebacondarwin

petebacondarwin May 17, 2017

Author Member

You're right. It is not needed.

This comment has been minimized.

@petebacondarwin

petebacondarwin May 17, 2017

Author Member

I guess that I should also import the AngularJS app module from mocks and use its name in line 19 below, rather than hard coding it.

constructor(i: Injector) { injector = i; }
}

export function createAngularTestingModule(angularJSModules: string[], strictDI = true) {

This comment has been minimized.

@gkalpak

gkalpak May 22, 2017

Member

Why string[]? angularJSModules can contain functions or arrays (which is not common in production code, but it is in tests).

This comment has been minimized.

@petebacondarwin

petebacondarwin May 24, 2017

Author Member

Fair point

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 3, 2017

Author Member

Actually, this is slightly different since we are not calling through to the ngMock.module() function but are actually calling the angular.module() function, which does indeed only expect an array of strings for the dependencies.

This comment has been minimized.

@gkalpak

gkalpak Jul 3, 2017

Member

AFAICT, it does accept functions or arrays 😕
(And they are useful for tests 😁)

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 3, 2017

Author Member

We are talking about this parameter: https://github.com/angular/angular.js/blob/master/src/loader.js#L75

Are you saying that one can do:

angular.module('app', [function($rootScope) { ... }]);

This comment has been minimized.

@gkalpak

gkalpak Jul 3, 2017

Member

Actually, you can, but I know realize that un-annotated functions will not work correctly when strict-di is, unless passed thought angular.mock.module.

I guess there is a reason for using angular.module directly, right?

import {$INJECTOR, INJECTOR_KEY} from '../common/constants';

export function createAngularJSTestingModule(AngularModules: any[]) {
return angular.module('$$angularJSTestingModule', [])

This comment has been minimized.

@gkalpak

gkalpak May 22, 2017

Member

Would it be possible/useful to use both createXyzTetingModule helpers in the same test (in which case we need different AngularJS module names)?

This comment has been minimized.

@petebacondarwin

petebacondarwin May 24, 2017

Author Member

I don't think you could/should use both helpers in the same test. One is designed for running tests from inside the AngularJS test infrastructure and the other from the Angular infrastructure.

import {createAngularJSTestingModule} from '@angular/upgrade/src/testing/create_angularjs_test_module';
import {AppModule, Inventory} from './mocks';

const ngMock = (window as any).angular.mock;

This comment has been minimized.

@gkalpak

gkalpak May 22, 2017

Member

Why not use the imported angular or even add mock as an optional property and import just that?

This comment has been minimized.

@petebacondarwin

petebacondarwin May 24, 2017

Author Member

Sadly we can't. I did try. The angular that we import is not actually the global. It is a local export from our common/angular1.ts file, which manually delegates to the global (once it has been "set" via setAngularJSLib()).

I couldn't come up with a simpler way to achieve this. Any ideas?

This comment has been minimized.

@gkalpak

gkalpak May 26, 2017

Member

We could expose mock (if present on the angular object) from angular1.ts. But given this is just for our tests and not needed in ngUpgrade itself, getting it from window.angular is fine imo.

This comment has been minimized.

@petebacondarwin

petebacondarwin May 26, 2017

Author Member

That was also my view too

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented May 22, 2017

As a reminder, the helpers need to be exported and upgrade/testing should be added to a bunch of lists for it to appear in the docs etc.

@juliemr

This comment has been minimized.

Copy link
Member

juliemr commented Jun 23, 2017

Ping?

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 25, 2017

Sorry I have been distracted. I will tidy it up this week.

@petebacondarwin petebacondarwin self-assigned this Jun 25, 2017

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch from c73b392 to 52735ee Jul 3, 2017

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jul 3, 2017

I have made the suggested changes, where appropriate, added documentation and included this new module in as many configuration files as I could find.

PTAL: @mhevery @juliemr @gkalpak

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jul 3, 2017

And of course this PR falls foul of the change from CircleCI v1 to v2.

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jul 3, 2017

Just checking to see if closing and reopening helps with CircleCI

@gkalpak
Copy link
Member

gkalpak left a comment

Just a few nits. Looks great so far (except that I have no idea about all the places we need to list a new package 😁)
I assume more tests will be added when this is finalized.

One thing I am a little concerned with is the namespacing of the module. Typically @angular/foo/testing is for testing @angular/foo/bar. So, I would expect the testing package for @angular/upgrade/static to be @angular/upgrade/static/testing (while @angular/upgrade/testing sounds more about testing @angular/upgrade).
I hope that eventually, @angular/upgrade will get dropped and @angular/upgrade/static will be moved to @angular/upgrade (together with its testing module).

@@ -57,6 +57,7 @@ module.exports = new Package('angular-api', [basePackage, typeScriptPackage])
'router/upgrade/index.ts',
'upgrade/index.ts',
'upgrade/static/index.ts',
'upgrade/testing/index.ts'

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

I wonder if upgrade/static/testing/ would be a more appropriate place for this (since it is upgrade/static/-specific.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 5, 2017

Author Member

But is it? All the code that we reference is in the common folder, so I believe that this testing module could be used for either dynamic or static. What do you think?

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Good point. From a quick look it seems you are right (in which case the name is fine) 😃

@@ -21,6 +21,7 @@ import * as platformServerTesting from '@angular/platform-server/testing';
import * as router from '@angular/router';
import * as routerTesting from '@angular/router/testing';
import * as upgrade from '@angular/upgrade';

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Unrelated to this PR: Does this include upgrade/static? Shouldn't it be explicitly imported?

* In the following code snippet, we are configuring the TestBed with two imports.
* The `AppModule` is the Angular part of our hybrid application and the `appModule` is the AngularJS part.
*
* ```

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Is the language of the snippet auto-detected?

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 5, 2017

Author Member

Not sure but I will add the language for good measure

* In the following code snippet, `shoppingCart` is an AngularJS service that depends upon an Angular service.
*
* ```
* it('should ...', inject(['shoppingCart', function(shoppingCart) {

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Is the inline array notation necessary for DI to work. I think angular.mock.inject can take care of that, even when strictDI is true.

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 5, 2017

Author Member

fair enough. I was trying to make it fail proof :-)

* ```
*
* @param angularJSModules a collection of the names of AngularJS modules to include in the configuration
* @param strictDI whether the AngularJS injector should have `strictDI` enabled. Defaults to `true`.

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Wrong params 😃

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 5, 2017

Author Member

😱

* @param angularJSModules a collection of the names of AngularJS modules to include in the configuration
* @param strictDI whether the AngularJS injector should have `strictDI` enabled. Defaults to `true`.
*/
export function createAngularJSTestingModule(AngularModules: any[]) {

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Why any[]. Can't we have a stricter type?

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 29, 2017

Author Member

The TestBed.configureTestingModule() function expects imports to be any[]

INJECTOR_KEY,
[
$INJECTOR,
($injector: any) => {

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Not that is makes any difference, but why any 😕

This comment has been minimized.

@petebacondarwin

petebacondarwin Jul 29, 2017

Author Member

fixed

@@ -0,0 +1,67 @@
/**

This comment has been minimized.

@gkalpak

gkalpak Jul 5, 2017

Member

Nit: We could keep the names consistent with the exported function names: _test_module --> _testing_module 😁

@mhevery mhevery force-pushed the angular:master branch from 08b36cb to 5d4b36f Jul 20, 2017

@kieranblight

This comment has been minimized.

Copy link

kieranblight commented Jul 21, 2017

Hi @petebacondarwin, I am just wondering what the rough timeline for this to be release will be?

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch from 0743841 to 0260634 Jul 22, 2017

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jul 22, 2017

@kieranblight - I have been having difficulties getting the new package into the system. I hope to finish this and get it in by the end of this week.

@kieranblight

This comment has been minimized.

Copy link

kieranblight commented Jul 24, 2017

@petebacondarwin thank you, look forward to it :)!

export function createAngularTestingModule(angularJSModules: string[], strictDI = true) {
angular.module('$$angularJSTestingModule', angularJSModules)
.factory(INJECTOR_KEY, () => injector);
$injector = angular.injector(['$$angularJSTestingModule'], strictDI);

This comment has been minimized.

@juliemr

juliemr Jul 25, 2017

Member

This should probably include ng as in:

$injector = angular.injector(['ng', '$$angularJSTestingModule'], strictDI);

So that createAngularTestingModule works without explicitly listing ng?

This comment has been minimized.

@gkalpak

gkalpak Jul 25, 2017

Member

For context:
This is automatically done when bootstrapping an app or by ngMock in tests. So we must "manually" do it here.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch 3 times, most recently from 0ca84ba to 733d0aa Jul 29, 2017

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch 2 times, most recently from d3c0ab5 to 590e8c2 Aug 16, 2017

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch from ba85f2e to c4c98e4 Oct 15, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Oct 15, 2018

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch from c4c98e4 to b9193f6 Oct 16, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Oct 16, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Oct 16, 2018

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch from 81184b9 to 5984a60 Oct 16, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Oct 16, 2018

($injector: angular.IInjectorService) => {
TestBed.configureTestingModule({
imports: angularModules,
providers: [{provide: $INJECTOR, useValue: $injector}]

This comment has been minimized.

@petebacondarwin

petebacondarwin Oct 29, 2018

Author Member

We cannot use useValue for this provider because the Angular compiler will try to parse the $injector object and crash. Instead useFactory works:

{provide: $INJECTOR, useFactory: () => $injector}
@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Oct 29, 2018

We should also provide an "injector only version" that does not require a full TestBed to be compiled. E.g. Something like this (thanks to @jbedard)

export function provideInjector(...providers: StaticProvider[]) {
    return function($provide: angular.auto.IProvideService) {
        $provide.factory("$$angularInjector", ["$injector", function($injector: angular.auto.IInjectorService) {
            return Injector.create([
                ...providers,
                {provide: "$injector", useValue: $injector}
            ]);
        }]);
        $provide.factory("$$angularLazyModuleRef", ["$$angularInjector", (injector) => ({injector, needsNgZone: true})]);
    };
}

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:ngupgrade-testing-api branch from 5984a60 to 7d285db Oct 30, 2018

@jasonaden jasonaden added this to the needsTriage milestone Jan 29, 2019

@theandrewlane

This comment has been minimized.

Copy link

theandrewlane commented Mar 5, 2019

Would be cool if this got merged in! 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.