Skip to content

refactor(schematics): constructor check rule does not check all diagnostics #13103

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions src/lib/schematics/update/material/data/property-names.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,21 @@ export interface MaterialPropertyNameData {
}

export const propertyNames: VersionChanges<MaterialPropertyNameData> = {
[TargetVersion.V7]: [
{
pr: 'https://github.com/angular/material2/pull/8286',
changes: [
{
replace: 'onChange',
replaceWith: 'changed',
whitelist: {
classes: ['SelectionModel']
}
}
]
}
],

[TargetVersion.V6]: [
{
pr: 'https://github.com/angular/material2/pull/10161',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ function visitSourceFile(context: WalkContext<null>, program: ts.Program) {
const node = findConstructorNode(diagnostic, sourceFile);

if (!node) {
return;
continue;
}

const classType = program.getTypeChecker().getTypeAtLocation(node.expression);
Expand All @@ -63,7 +63,7 @@ function visitSourceFile(context: WalkContext<null>, program: ts.Program) {
// TODO(devversion): Consider handling pass-through classes better.
// TODO(devversion): e.g. `export class CustomCalendar extends MatCalendar {}`
if (!constructorChecks.includes(className)) {
return;
continue;
}

const classSignatures = classType.getConstructSignatures()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ describe('v5 constructor checks', () => {

await runPostScheduledTasks(runner, 'tslint-fix').toPromise();

expect(output).toMatch(/\[22.*Found "NativeDateAdapter"/,
'Expected the constructor checks to report if an argument is not assignable.');
expect(output).not.toMatch(/\[26.*Found "NativeDateAdapter".*/,
'Expected the constructor to not report if an argument is assignable.');

expect(output).not.toMatch(/Found "NonMaterialClass".*: new NonMaterialClass\(\)/);

expect(output).toMatch(/Found "NativeDateAdapter".*super.*: super\(string, Platform\)/);
expect(output).toMatch(/Found "NativeDateAdapter".*: new \w+\(string, Platform\)/);

Expand All @@ -31,12 +38,11 @@ describe('v5 constructor checks', () => {
expect(output).toMatch(/Found "MatCalendar".*super.*: super\(any, any, any, any\)/);
expect(output).toMatch(/Found "MatCalendar".*: new \w+\(any, any, any, any\)/);

expect(output).toMatch(/\[97.*Found "NativeDateAdapter"/,
'Expected the constructor checks to report if an argument is not assignable.');
expect(output).not.toMatch(/\[99.*Found "NativeDateAdapter".*/,
'Expected the constructor to not report if an argument is assignable.');
expect(output).toMatch(/Found "MatDrawerContent".*super.*: super\((any, ){4}any\)/);
expect(output).toMatch(/Found "MatDrawerContent".*: new \w+\((any, ){4}any\)/);

expect(output).not.toMatch(/Found "NonMaterialClass".*: new NonMaterialClass\(\)/);
expect(output).toMatch(/Found "MatSidenavContent".*super.*: super\((any, ){4}any\)/);
expect(output).toMatch(/Found "MatSidenavContent".*: new \w+\((any, ){4}any\)/);
});
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,14 @@ class MatAutocomplete {
constructor(_changeDetector: any, _elementRef: any, _defaults: string[]) {}
}

class NonMaterialClass {}

const _t1 = new NativeDateAdapter('b', 'invalid-argument');

const _t2 = new NativeDateAdapter('a', {IOS: true});

const _t3 = new NonMaterialClass('invalid-argument');

class MatTooltip {
constructor(
private _overlay: any,
Expand All @@ -40,10 +48,20 @@ class MatCalendar {
constructor(_intl: any, _adapter: any, _formats: any, _changeDetector: any) {}
}

class NonMaterialClass {}
class MatDrawerContent {
constructor (_cd: any,
_container: any,
_elementRef: any,
_scrollDispatcher: any,
_ngZone: any) {}
}

class DelegateClass {
constructor(_adapter: NativeDateAdapter) {}
class MatSidenavContent {
constructor (_cd: any,
_container: any,
_elementRef: any,
_scrollDispatcher: any,
_ngZone: any) {}
}

/* Actual test case using the previously defined definitions. */
Expand Down Expand Up @@ -94,8 +112,18 @@ class E extends MatCalendar {

const _E = new MatCalendar({}, {}, {}, {}, {}, {}, {});

const _F = new NativeDateAdapter('b', 'invalid-argument');
class F extends MatDrawerContent {
constructor(changeDetectorRef: any, container: any) {
super(changeDetectorRef, container);
}
}

const _F = new MatDrawerContent({}, 'container');

const _G = new NativeDateAdapter('a', {IOS: true});
class G extends MatSidenavContent {
constructor(changeDetectorRef: any, container: any) {
super(changeDetectorRef, container);
}
}

const _H = new NonMaterialClass('invalid-argument');
const _G = new MatSidenavContent({}, 'container');
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,20 @@ import {runPostScheduledTasks} from '../../test-setup/post-scheduled-tasks';
import {migrationCollection} from '../../test-setup/test-app';
import {createTestAppWithTestCase, readFileContent, resolveBazelDataFile} from './index.spec';

describe('v5 test cases', () => {
describe('v6 upgrade test cases', () => {

/**
* Name of test cases that will be used to verify that update schematics properly update
* a developers application.
*/
const testCases = [
'v5/attribute-selectors',
'v5/class-names',
'v5/css-names',
'v5/element-selectors',
'v5/input-names',
'v5/output-names',
'v5/property-names',
'v6/attribute-selectors',
'v6/class-names',
'v6/css-names',
'v6/element-selectors',
'v6/input-names',
'v6/output-names',
'v6/property-names',
];

// Iterates through every test case directory and generates a jasmine test block that will
Expand Down
38 changes: 38 additions & 0 deletions src/lib/schematics/update/test-cases/v7-test-cases.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import {SchematicTestRunner} from '@angular-devkit/schematics/testing';
import {runPostScheduledTasks} from '../../test-setup/post-scheduled-tasks';
import {migrationCollection} from '../../test-setup/test-app';
import {createTestAppWithTestCase, readFileContent, resolveBazelDataFile} from './index.spec';

describe('v7 upgrade test cases', () => {

/**
* Name of test cases that will be used to verify that update schematics properly update
* a developers application.
*/
const testCases = [
'v7/property-names'
];

// Iterates through every test case directory and generates a jasmine test block that will
// verify that the update schematics properly update the test input to the expected output.
testCases.forEach(testCaseName => {
const inputPath = resolveBazelDataFile(`${testCaseName}_input.ts`);
const expectedOutputPath = resolveBazelDataFile(`${testCaseName}_expected_output.ts`);

it(`should apply update schematics to test case: ${testCaseName}`, () => {
const runner = new SchematicTestRunner('schematics', migrationCollection);

runner.runSchematic('migration-02', {}, createTestAppWithTestCase(inputPath));

// Run the scheduled TSLint fix task from the update schematic. This task is responsible for
// identifying outdated code parts and performs the fixes. Since tasks won't run automatically
// within a `SchematicTestRunner`, we manually need to run the scheduled task.
return runPostScheduledTasks(runner, 'tslint-fix').toPromise().then(() => {
expect(readFileContent('projects/material/src/main.ts'))
.toBe(readFileContent(expectedOutputPath));
});
});
});
});


Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import {EventEmitter, OnInit} from '@angular/core';

class SelectionModel {
onChange = new EventEmitter<void>();
}

/* Actual test case using the previously defined definitions. */

class A implements OnInit {
self = {me: this};

constructor(private a: SelectionModel) {}

ngOnInit() {
this.a.changed.subscribe(() => console.log('Changed'));
this.self.me.a.changed.subscribe(() => console.log('Changed 2'));
}
}
18 changes: 18 additions & 0 deletions src/lib/schematics/update/test-cases/v7/property-names_input.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import {EventEmitter, OnInit} from '@angular/core';

class SelectionModel {
onChange = new EventEmitter<void>();
}

/* Actual test case using the previously defined definitions. */

class A implements OnInit {
self = {me: this};

constructor(private a: SelectionModel) {}

ngOnInit() {
this.a.onChange.subscribe(() => console.log('Changed'));
this.self.me.a.onChange.subscribe(() => console.log('Changed 2'));
}
}