build: add test cases for update schematics #12473

devversion · 2018-08-01T15:35:02Z

Adds a way to test that the update schematics properly update a developer's application.
As a start, we test the classNames data for the V5 to V6 upgrade.

devversion · 2018-08-01T16:26:05Z

What I plan to do in the future in regards to this PR:

When having multiple fixtures --> do not always re-create file system
Push for a public API on the devkit to run the schematic tasks within the SchematicTestRunner.
Slowly add more fixtures for testing. Might involve switching more rules away from type checking

jelbourn · 2018-08-01T16:17:55Z

src/lib/schematics/update/tests/class-names/expected.fixture

@@ -0,0 +1,14 @@
+import {CdkConnectedOverlay, CdkOverlayOrigin} from '@angular/cdk/overlay';


I don't think "fixture" is the right word for this. I'd also like to keep the .ts extension so that the files get the right syntax highlighting everywhere.

How about

schematics/update/test-cases/ts-class-name.before.ts schematics/update/test-cases/ts-class-name.after.ts

Other possible pairs could be "input" / "output"

cc @hansl in case he has any ideas for this

input and output sounds better IMO. I'd even go with input and expected_output.

In regards to the extension. Technically it's not real valid TypeScript code because the imports are not valid, and just to mock the Material and CDK packages. I don't really care that much though.

I like input and expected_output

Even if the imports are red, I like having the syntax highlighting.

That's reasonable. Will do the changes.

jelbourn · 2018-08-01T16:20:11Z

src/lib/schematics/update/tests/run-fixtures.spec.ts

+   * will be used to verify the update schematic.
+   */
+  const fixturesDirectories = [
+    'angular_material/src/lib/schematics/update/tests/class-names',


Should this use a glob? Seems like it would be a pain to keep this up to date.

This won't work for now because Bazel does not copy the files that are referenced in the data attribute of the jasmine_node_test rule. I'll add a TODO and revisit once we found a better solution.

jelbourn · 2018-08-01T16:51:37Z

src/lib/schematics/update/update.ts

  return () => console.log(
      '\nComplete! Please check the output above for any issues that were detected but could not' +
      ' be automatically fixed.');
 }

-/** Gets the first tsconfig path from possibile locations based on the history of the CLI. */
+/** Gets the first tsconfig path from possible locations based on the history of the CLI. */


Could you update this comment so that it says "Gets all tsconfig..." instead of the "Gets the first..."

jelbourn · 2018-08-01T16:58:16Z

src/lib/schematics/utils/testing.ts

+export function runPostScheduledTasks(runner: SchematicTestRunner, taskName: string)
+    : Observable<void> {
+
+  const host = runner.engine['_host'] as EngineHost<{}, {}>;


Can you link here to the github issue for exposing these once you file it?

devversion · 2018-08-01T18:32:10Z

@jelbourn Made the requested changes. Please have another look 😄

devversion · 2018-08-01T18:35:11Z

tslint.json

@@ -119,7 +119,7 @@
    }, "src/+(lib|cdk|material-experimental|cdk-experimental)/**/!(*.spec).ts"],
    "require-license-banner": [
      true,
-      "src/+(lib|cdk|material-experimental|cdk-experimental|demo-app)/**/!(*.spec).ts"
+      "src/+(lib|cdk|material-experimental|cdk-experimental|demo-app)/**/!(*.spec|*.fixture).ts"


Actually, I need to find a way to properly exclude the test cases here. Will work on that

jelbourn · 2018-08-01T19:38:15Z

src/lib/schematics/update/tests/class-names/input.ts

@@ -0,0 +1,14 @@
+import {ConnectedOverlayDirective, OverlayOrigin} from '@angular/cdk/overlay';


Should we be including the version in here as well? E.g., this test case is based on v5, but that's not captured anywhere. I'm thinking we could do something like:

schematics/update/test-cases/v5/ts-class-name_input.ts schematics/update/test-cases/v5/ts-class-name_expected_output.ts

That's a good question. I'm not too sure yet -- I actually wouldn't differentiate between V5 and V6 because otherwise it wouldn't be really verifying how the tool will work in CLI projects that are on V5 and just run ng update.

Also it kind of depends on how we handle the V5 and V6 data (e.g. whether we merge the data; or have separate rules; or separate schematics).

I'd personally vote for a follow-up. PR. The PR is already pretty big, includes lots of workarounds and is primarily about the infrastructure and ability to run such test cases.

Ah, interesting. I had always assumed we would only run the v5 -> v6 rules when the user is on v5, and then the same for v6 -> v7 and so on. Now I realize, though, it's set up to run all of them all the time.

As far as correctness goes, I don't think it matters; the symbols from v5 should never come back, so the old rules would be a no-op. However, I would be slightly concerned that there would be a performance cost to running all of the rules for old versions (in making ng update take longer). I'd want to try running them over an app or two to gauge the cost.

Also cc @hansl for input

That's true. The question is just how much code we'd need to duplicate in order to differentiate between an update from v5 or an update from v6.

Since we run the rules through the TslintFixTask, we have no real way of telling the rules from the schematic, that the update starts with v5 or v6.

Surely there would be other ways, but not sure how much we gain from the extra effort. It's not like we have a lot of rules. From my investigations, all of them still run pretty fast.

--
Also, as far as I could tell, there is not really a way to identify the version of Material before the update because the CLI performs the npm update before running the schematic.

(e.g. options are not passed to the migration schematics; see here)

@crisbeto @mmalerba @josephperrott @vivian-hu do you have any opinions on file organization here?

it wouldn't be really verifying how the tool will work in CLI projects that are on V5 and just run ng update.

The CLI will run all migrations between [from, to] (inclusive), in order. So you will run the v5 -> v6 migration, then whatever is after v6 (I would presume v6 -> v7 migration).

I need to get you off tslint asap. Or you need to get off of it yourself :) . It just complicates things.

@hansl I see 😄 I didn't really integrate the TSLint rules into the schematics. At some point the reasoning for the TSLint rules in the initial standalone tool was that the rules could be run inside of Google. But not sure how relevant this is today.

What would you recommend for doing the update tool? Just using the TS API without TSLint?

For now, that's unfortunately the best.

@jelbourn I've just made the changes from this comment. As mentioned above, I see the point of having a v5 separation, if we have multiple entry-points for the migration schematics.

* Adds a way to test that the update schematics properly update a developer's application. * As a start, we test the `classNames` data for the V5 to V6 upgrade.

jelbourn

LGTM

angular-automatic-lock-bot · 2019-09-09T09:01:26Z

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.}

devversion added pr: merge safe target: major This PR is targeted for the next major release labels Aug 1, 2018

devversion assigned jelbourn Aug 1, 2018

devversion requested review from amcdnl and jelbourn as code owners August 1, 2018 15:35

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 1, 2018

devversion added the in progress This issue is currently in progress label Aug 1, 2018

jelbourn reviewed Aug 1, 2018

View reviewed changes

devversion force-pushed the build/schematics-update-tests-tslint branch from c38c72c to c2ec30a Compare August 1, 2018 18:31

devversion changed the title ~~build: add fixture testing for update schematics~~ build: add test cases for update schematics Aug 1, 2018

devversion commented Aug 1, 2018

View reviewed changes

jelbourn reviewed Aug 1, 2018

View reviewed changes

devversion force-pushed the build/schematics-update-tests-tslint branch 3 times, most recently from 501cf44 to 68cb8fa Compare August 2, 2018 17:17

build: add test cases for update schematics

69a3840

* Adds a way to test that the update schematics properly update a developer's application. * As a start, we test the `classNames` data for the V5 to V6 upgrade.

devversion force-pushed the build/schematics-update-tests-tslint branch from 68cb8fa to 69a3840 Compare August 2, 2018 17:18

jelbourn approved these changes Aug 2, 2018

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Aug 2, 2018

jelbourn merged commit 8cbe812 into angular:master Aug 2, 2018

devversion deleted the build/schematics-update-tests-tslint branch August 2, 2018 18:28

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

		@@ -0,0 +1,14 @@
		import {CdkConnectedOverlay, CdkOverlayOrigin} from '@angular/cdk/overlay';

		@@ -0,0 +1,14 @@
		import {ConnectedOverlayDirective, OverlayOrigin} from '@angular/cdk/overlay';

build: add test cases for update schematics #12473

build: add test cases for update schematics #12473

Uh oh!

Conversation

devversion commented Aug 1, 2018

Uh oh!

devversion commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

devversion commented Aug 1, 2018 •

edited

Loading

devversion commented Aug 1, 2018 •

edited

Loading

jelbourn Aug 1, 2018 •

edited

Loading

devversion Aug 1, 2018 •

edited

Loading

devversion Aug 1, 2018 •

edited

Loading

jelbourn Aug 1, 2018 •

edited

Loading