fix(new): include routing in spec and inline template when called with `--routing` #3252

Meligy · 2016-11-23T13:59:22Z

This is kind-of missed work from #2794.

In that other PR, we prevented a browser error that happens when a user just runs:

ng new sample --routing
cd sample
ng serve

This works great now after #2794 was cherry picked.
However, there were 2 missing items:

ng test fails
Because the app.component has <router-outlet>, but the test does not import a routing module
The browser error still comes if you use --inline-template with ng new --routing
Because we only fixed app.component.html, but not the actual template in app.component.ts

This PR addresses these 2 issues.

Tests

I am really keen to add some tests to this PR.
Acceptance tests are not applicable here because they only check created files not file content, and I could not find any E2E tests for ng new that I could use for inspiration, or I could extend to cover this scenario.

Update: Thanks Hans for guidance. Tests were added.

hansl

LGTM, just one nit to add.

hansl · 2016-11-23T18:26:21Z

packages/angular-cli/blueprints/ng2/files/__path__/app/app.component.spec.ts

+    TestBed.configureTestingModule({<% if (routing) { %>
+      imports: [
+        RouterTestingModule
+      ],<% } %>


While we're fixing this, can you add a compileComponents call in here as well?

hansl · 2016-11-23T18:27:52Z

Oh and before I forget, seems like we missed this because we're missing an e2e test. Could you add one?

Meligy · 2016-11-25T10:29:43Z

@hansl I attempted to add an e2e test for ng new. It's very hard because all e2e tests share a parent project that's a special variation of ng new. Let me know what you think of it.

Meligy · 2016-11-25T18:30:29Z

I made the test specific for ng new --routing, since just ng new is kinda tested in all the tests (because it's called in setup. I say kinda because it's with specific non-default arguments but still). Also, this makes it more relevant to what the PR addresses.

hansl

Some more comments.

hansl · 2016-11-25T22:56:28Z

tests/e2e/utils/fs.ts

@@ -57,6 +57,27 @@ export function createDir(path: string) {
  _recursiveMkDir(path);
 }

+export function deleteDir(path: string) {
+  return Promise.resolve()
+      .then(() => _recursiveRmDir(path));


Use rimraf here (it's already in the dependencies, documentation here: https://github.com/isaacs/rimraf).

Actually we don't need it to make this e2e test, so let's just kill it :)

hansl · 2016-11-25T22:57:03Z

packages/angular-cli/blueprints/ng2/files/__path__/app/app.component.spec.ts

+    TestBed.configureTestingModule({<% if (routing) { %>
+      imports: [
+        RouterTestingModule
+      ],<% } %>


hansl · 2016-11-25T23:05:23Z

tests/e2e/tests/commands/new/new-routing.ts

+  const parentTestProjectDir = process.cwd();
+  const ngNewProjectName = 'new-test-project';
+
+  return Promise.resolve()


You seem to overly complicate how this test could work. Why not:

return return Promise.resolve() .then(() => process.chdir(getGlobalVariable('tmp-root'))) .then(() => ng('new', 'routing-project', '--routing')) .then(() => process.chdir(path.join('routing-project'))) .then(() => ng('test', '--single-run'));

The only thing missing would be to chdir back to the directory we were in before this test, but really e2e_runner should take care of that (and I'm going to do a PR for it).

We already work off a temporary directory, there's no need to clean up after yourself :) This is by design.

#3287 will fix the chdir.

You keep showing me awesome gems. I didn't realise the getGlobalVariable() before!

Will update tomorrow or today if I have time.

Meligy · 2016-11-27T05:25:54Z

@hansl I just made the change. The build will fail though until we get #3287.

…h `--routing`

…d from `ng new`

Meligy · 2016-12-01T09:10:51Z

Aaand we finally got a green build!

@hansl do you think this PR can be merged now? Anything else you'd think is needed?
Thanks heaps :)

hansl · 2016-12-01T19:36:24Z

LGTM! Thanks!

…th `--routing` (angular#3252)

…ular-cli#3252

angular-automatic-lock-bot · 2019-09-12T11:41:05Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes label Nov 23, 2016

Meligy force-pushed the fix/new-routing-inline-spec branch 2 times, most recently from d0e7918 to da5823e Compare November 23, 2016 14:05

Meligy changed the title ~~fix(ng new): include routing in spec and inline component template when using --routing flag~~ fix(new): include routing in spec and inline template when called with --routing Nov 23, 2016

Meligy force-pushed the fix/new-routing-inline-spec branch from da5823e to 4c7f8eb Compare November 23, 2016 18:00

hansl suggested changes Nov 23, 2016

View reviewed changes

Meligy force-pushed the fix/new-routing-inline-spec branch from 4c7f8eb to 6854bb5 Compare November 25, 2016 10:27

Meligy force-pushed the fix/new-routing-inline-spec branch 2 times, most recently from 2bea7bc to 06f85cb Compare November 25, 2016 18:27

Meligy force-pushed the fix/new-routing-inline-spec branch from 06f85cb to 2131fbf Compare November 25, 2016 18:32

hansl suggested changes Nov 25, 2016

View reviewed changes

Meligy force-pushed the fix/new-routing-inline-spec branch from 2131fbf to 8189d8f Compare November 27, 2016 05:21

Meligy force-pushed the fix/new-routing-inline-spec branch 2 times, most recently from 27a3651 to d9afc7e Compare December 1, 2016 00:04

Meligy mentioned this pull request Dec 1, 2016

'router-outlet' is not a known element #3331

Closed

Meligy added 3 commits December 1, 2016 15:50

fix(new): include routing in spec and inline template when called wit…

b8fe983

…h `--routing`

feat(new): call compileComponents(); in app component spec generate…

d44b91a

…d from `ng new`

chore(new): add e2e test for ng new --routing

f09f06c

Meligy force-pushed the fix/new-routing-inline-spec branch from d9afc7e to f09f06c Compare December 1, 2016 04:50

hansl approved these changes Dec 1, 2016

View reviewed changes

hansl added the pr_action: merge label Dec 1, 2016

hansl merged commit 53ab4df into angular:master Dec 1, 2016

jupapios mentioned this pull request Dec 10, 2016

Can not pass the test with newly initiated project #3233

Closed

MRHarrison pushed a commit to MRHarrison/angular-cli that referenced this pull request Feb 9, 2017

feat(new): include routing in spec and inline template when called wi…

12575ef

…th `--routing` (angular#3252)

mokipedia mentioned this pull request Jul 20, 2017

when running ng test - got a 'router-outlet' is not a known element - error - Angular 4 coreui/coreui-free-bootstrap-admin-template#202

Closed

amercier added a commit to amercier/angular-tour-of-heroes that referenced this pull request May 4, 2018

Imported RouterTestingModule in AppComponent specs as per angular/ang…

c025bdd

…ular-cli#3252

amercier added a commit to amercier/angular-tour-of-heroes that referenced this pull request May 4, 2018

Imported RouterTestingModule in AppComponent specs as per angular/ang…

b69ab9f

…ular-cli#3252

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(new): include routing in spec and inline template when called with `--routing` #3252

fix(new): include routing in spec and inline template when called with `--routing` #3252

Meligy commented Nov 23, 2016 •

edited

Loading

hansl left a comment

hansl Nov 23, 2016

hansl Nov 25, 2016

hansl commented Nov 23, 2016

Meligy commented Nov 25, 2016

Meligy commented Nov 25, 2016

hansl left a comment

hansl Nov 25, 2016

hansl Nov 25, 2016

hansl Nov 25, 2016

hansl Nov 25, 2016

hansl Nov 25, 2016

Meligy Nov 27, 2016

Meligy commented Nov 27, 2016 •

edited

Loading

Meligy commented Dec 1, 2016

hansl commented Dec 1, 2016

angular-automatic-lock-bot bot commented Sep 12, 2019

fix(new): include routing in spec and inline template when called with --routing #3252

fix(new): include routing in spec and inline template when called with --routing #3252

Conversation

Meligy commented Nov 23, 2016 • edited Loading

Tests

hansl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansl commented Nov 23, 2016

Meligy commented Nov 25, 2016

Meligy commented Nov 25, 2016

hansl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Meligy commented Nov 27, 2016 • edited Loading

Meligy commented Dec 1, 2016

hansl commented Dec 1, 2016

angular-automatic-lock-bot bot commented Sep 12, 2019

fix(new): include routing in spec and inline template when called with `--routing` #3252

fix(new): include routing in spec and inline template when called with `--routing` #3252

Meligy commented Nov 23, 2016 •

edited

Loading

Meligy commented Nov 27, 2016 •

edited

Loading