X Tutup
Skip to content

Optimize unit tests #5299

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.

Sign up for GitHub

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

Closed
vsavkin wants to merge 1 commit into from
Closed

Optimize unit tests #5299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
27 changes: 17 additions & 10 deletions gulpfile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,14 @@ if (cliArgs.projects) {

// --projects=angular2,angular2_material => {angular2: true, angular2_material: true}
var allProjects =
'angular1_router,angular2,angular2_material,benchmarks,benchmarks_external,benchpress,playground';
'angular1_router,angular2,angular2_material,benchmarks,benchmarks_external,benchpress,playground,bundle_deps';
var cliArgsProjects = (cliArgs.projects || allProjects)
.split(',')
.reduce((map, projectName) => {
map[projectName] = true;
return map;
}, {});
var generateEs6 = !cliArgs.projects;

function printModulesWarning() {
if (!cliArgs.projects && !process.env.CI) {
Expand Down Expand Up @@ -302,11 +303,11 @@ gulp.task('lint', ['build.tools'], function() {
// ------------
// check circular dependencies in Node.js context
gulp.task('build/checkCircularDependencies', function(done) {
var dependencyObject = madge(CONFIG.dest.js.dev.es6, {
format: 'es6',
paths: [CONFIG.dest.js.dev.es6],
var dependencyObject = madge(CONFIG.dest.js.dev.es5, {
format: 'cjs',
paths: [CONFIG.dest.js.dev.es5],
extensions: ['.js'],
onParseFile: function(data) { data.src = data.src.replace(/import \* as/g, "//import * as"); }
onParseFile: function(data) { data.src = data.src.replace(/\/\* circular \*\//g, "//"); }
Copy link
Copy Markdown
Contributor

@alexeagle alexeagle Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cheater!!

Copy link
Copy Markdown
Contributor Author

@vsavkin vsavkin Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no other way to do it :) The right way to do the check is to use cjs sources, not es6, and there we cannot rely on the import as trick.

Copy link
Copy Markdown
Contributor Author

@vsavkin vsavkin Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is interesting that we have only one place in our code base where this is necessary. Maybe we can restructure our code to get rid of it.

Copy link
Copy Markdown
Contributor

@alexeagle alexeagle Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a while on the other way to do it, and would love to show you :)
Will you be in the office today?

On Mon, Nov 16, 2015 at 9:20 AM Victor Savkin notifications@github.com
wrote:

In gulpfile.js
#5299 (comment):

 extensions: ['.js'],
  • onParseFile: function(data) { data.src = data.src.replace(/import * as/g, "//import * as"); }
  • onParseFile: function(data) { data.src = data.src.replace(//* circular *//g, "//"); }

There is no other way to do it :) The right way to do the check is to use
cjs sources, not es6, and there we cannot rely on the import as trick.

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular/pull/5299/files#r44952906.

Copy link
Copy Markdown
Contributor

@alexeagle alexeagle Nov 16, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worked with Tobias on that circular dep. I think we should remove it by
making it a type-checker-only dependency. I'll show you.
Here was my work on this: #4454

On Mon, Nov 16, 2015 at 9:21 AM Alex Eagle alexeagle@google.com wrote:

I spent a while on the other way to do it, and would love to show you :)
Will you be in the office today?

On Mon, Nov 16, 2015 at 9:20 AM Victor Savkin notifications@github.com
wrote:

In gulpfile.js
#5299 (comment):

 extensions: ['.js'],
  • onParseFile: function(data) { data.src = data.src.replace(/import * as/g, "//import * as"); }
  • onParseFile: function(data) { data.src = data.src.replace(//* circular *//g, "//"); }

There is no other way to do it :) The right way to do the check is to use
cjs sources, not es6, and there we cannot rely on the import as trick.

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular/pull/5299/files#r44952906.

});
var circularDependencies = dependencyObject.circular().getArray();
if (circularDependencies.length > 0) {
Expand Down Expand Up @@ -841,11 +842,15 @@ gulp.task('!build.tools', function() {
gulp.task('broccoli.js.dev', ['build.tools'],
function(done) { runSequence('!broccoli.js.dev', sequenceComplete(done)); });

gulp.task('!broccoli.js.dev',
() => { return angularBuilder.rebuildBrowserDevTree(cliArgsProjects); });
gulp.task(
'!broccoli.js.dev',
() => angularBuilder.rebuildBrowserDevTree(
{generateEs6: generateEs6, projects: cliArgsProjects, noTypeChecks: cliArgs.noTypeChecks}));

gulp.task('!broccoli.js.prod',
function() { return angularBuilder.rebuildBrowserProdTree(cliArgsProjects); });
gulp.task(
'!broccoli.js.prod',
() => angularBuilder.rebuildBrowserProdTree(
{generateEs6: generateEs6, projects: cliArgsProjects, noTypeChecks: cliArgs.noTypeChecks}));

gulp.task('build.js.dev', ['build/clean.js'], function(done) {
runSequence('broccoli.js.dev', 'build.css.material', sequenceComplete(done));
Expand All @@ -868,7 +873,9 @@ var firstBuildJsCjs = true;
* private task
*/
gulp.task('!build.js.cjs', function() {
return angularBuilder.rebuildNodeTree(cliArgsProjects)
return angularBuilder
.rebuildNodeTree(
{generateEs6: generateEs6, projects: cliArgsProjects, noTypeChecks: cliArgs.noTypeChecks})
.then(function() {
if (firstBuildJsCjs) {
firstBuildJsCjs = false;
Expand Down
5 changes: 3 additions & 2 deletions modules/angular2/src/common/forms/directives/checkbox_value_accessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {Self, forwardRef, Provider} from 'angular2/src/core/di';

import {NG_VALUE_ACCESSOR, ControlValueAccessor} from './control_value_accessor';
import {CONST_EXPR} from 'angular2/src/facade/lang';
import {setProperty} from './shared';

const CHECKBOX_VALUE_ACCESSOR = CONST_EXPR(new Provider(
NG_VALUE_ACCESSOR, {useExisting: forwardRef(() => CheckboxControlValueAccessor), multi: true}));
Expand All @@ -30,7 +29,9 @@ export class CheckboxControlValueAccessor implements ControlValueAccessor {

constructor(private _renderer: Renderer, private _elementRef: ElementRef) {}

writeValue(value: any): void { setProperty(this._renderer, this._elementRef, "checked", value); }
writeValue(value: any): void {
this._renderer.setElementProperty(this._elementRef, 'checked', value);
}
registerOnChange(fn: (_: any) => {}): void { this.onChange = fn; }
registerOnTouched(fn: () => {}): void { this.onTouched = fn; }
}
5 changes: 2 additions & 3 deletions modules/angular2/src/common/forms/directives/default_value_accessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {Renderer} from 'angular2/src/core/render';
import {Self, forwardRef, Provider} from 'angular2/src/core/di';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from './control_value_accessor';
import {isBlank, CONST_EXPR} from 'angular2/src/facade/lang';
import {setProperty} from './shared';

const DEFAULT_VALUE_ACCESSOR = CONST_EXPR(new Provider(
NG_VALUE_ACCESSOR, {useExisting: forwardRef(() => DefaultValueAccessor), multi: true}));
Expand Down Expand Up @@ -39,9 +38,9 @@ export class DefaultValueAccessor implements ControlValueAccessor {

writeValue(value: any): void {
var normalizedValue = isBlank(value) ? '' : value;
setProperty(this._renderer, this._elementRef, 'value', normalizedValue);
this._renderer.setElementProperty(this._elementRef, 'value', normalizedValue);
}

registerOnChange(fn: (_: any) => void): void { this.onChange = fn; }
registerOnTouched(fn: () => void): void { this.onTouched = fn; }
}
}
5 changes: 3 additions & 2 deletions modules/angular2/src/common/forms/directives/number_value_accessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import {Renderer} from 'angular2/src/core/render';
import {Self, forwardRef, Provider} from 'angular2/src/core/di';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from './control_value_accessor';
import {isBlank, CONST_EXPR, NumberWrapper} from 'angular2/src/facade/lang';
import {setProperty} from './shared';

const NUMBER_VALUE_ACCESSOR = CONST_EXPR(new Provider(
NG_VALUE_ACCESSOR, {useExisting: forwardRef(() => NumberValueAccessor), multi: true}));
Expand Down Expand Up @@ -34,7 +33,9 @@ export class NumberValueAccessor implements ControlValueAccessor {

constructor(private _renderer: Renderer, private _elementRef: ElementRef) {}

writeValue(value: number): void { setProperty(this._renderer, this._elementRef, 'value', value); }
writeValue(value: number): void {
this._renderer.setElementProperty(this._elementRef, 'value', value);
}

registerOnChange(fn: (_: number) => void): void {
this.onChange = (value) => { fn(NumberWrapper.parseFloat(value)); };
Expand Down
3 changes: 1 addition & 2 deletions modules/angular2/src/common/forms/directives/select_control_value_accessor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {Query, Directive} from 'angular2/src/core/metadata';
import {ObservableWrapper} from 'angular2/src/facade/async';
import {NG_VALUE_ACCESSOR, ControlValueAccessor} from './control_value_accessor';
import {CONST_EXPR} from 'angular2/src/facade/lang';
import {setProperty} from './shared';

const SELECT_VALUE_ACCESSOR = CONST_EXPR(new Provider(
NG_VALUE_ACCESSOR, {useExisting: forwardRef(() => SelectControlValueAccessor), multi: true}));
Expand Down Expand Up @@ -50,7 +49,7 @@ export class SelectControlValueAccessor implements ControlValueAccessor {

writeValue(value: any): void {
this.value = value;
setProperty(this._renderer, this._elementRef, "value", value);
this._renderer.setElementProperty(this._elementRef, 'value', value);
}

registerOnChange(fn: () => any): void { this.onChange = fn; }
Expand Down
7 changes: 0 additions & 7 deletions modules/angular2/src/common/forms/directives/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import {NgControlGroup} from './ng_control_group';
import {Control, ControlGroup} from '../model';
import {Validators} from '../validators';
import {ControlValueAccessor} from './control_value_accessor';
import {ElementRef, QueryList} from 'angular2/src/core/linker';
import {Renderer} from 'angular2/src/core/render';
import {DefaultValueAccessor} from './default_value_accessor';
import {NumberValueAccessor} from './number_value_accessor';
import {CheckboxControlValueAccessor} from './checkbox_value_accessor';
Expand Down Expand Up @@ -57,11 +55,6 @@ function _throwError(dir: AbstractControlDirective, message: string): void {
throw new BaseException(`${message} '${path}'`);
}

export function setProperty(renderer: Renderer, elementRef: ElementRef, propName: string,
propValue: any) {
renderer.setElementProperty(elementRef, propName, propValue);
}

export function composeValidators(validators: /* Array<Validator|Function> */ any[]): Function {
return isPresent(validators) ? Validators.compose(validators.map(normalizeValidator)) : null;
}
Expand Down
20 changes: 10 additions & 10 deletions modules/angular2/src/core/linker/element_injector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,8 @@ import {resolveProvider, ResolvedFactory, ResolvedProvider_} from 'angular2/src/

import {AttributeMetadata, QueryMetadata} from '../metadata/di';

import * as viewModule from './view';
import * as avmModule from './view_manager';
import {AppViewContainer, AppView} from './view';
/* circular */ import * as avmModule from './view_manager';
import {ViewContainerRef} from './view_container_ref';
import {ElementRef} from './element_ref';
import {TemplateRef} from './template_ref';
Expand Down Expand Up @@ -183,8 +183,8 @@ export class DirectiveProvider extends ResolvedProvider_ {

// TODO(rado): benchmark and consider rolling in as ElementInjector fields.
export class PreBuiltObjects {
nestedView: viewModule.AppView = null;
constructor(public viewManager: avmModule.AppViewManager, public view: viewModule.AppView,
nestedView: AppView = null;
constructor(public viewManager: avmModule.AppViewManager, public view: AppView,
public elementRef: ElementRef, public templateRef: TemplateRef) {}
}

Expand All @@ -195,7 +195,7 @@ export class QueryMetadataWithSetter {
export class EventEmitterAccessor {
constructor(public eventName: string, public getter: Function) {}

subscribe(view: viewModule.AppView, boundElementIndex: number, directive: Object): Object {
subscribe(view: AppView, boundElementIndex: number, directive: Object): Object {
var eventEmitter = this.getter(directive);
return ObservableWrapper.subscribe<Event>(
eventEmitter,
Expand Down Expand Up @@ -235,7 +235,7 @@ function _createProtoQueryRefs(providers: ProviderWithVisibility[]): ProtoQueryR
}

export class ProtoElementInjector {
view: viewModule.AppView;
view: AppView;
attributes: Map<string, string>;
eventEmitterAccessors: EventEmitterAccessor[][];
protoQueryRefs: ProtoQueryRef[];
Expand Down Expand Up @@ -451,9 +451,9 @@ export class ElementInjector extends TreeNode<ElementInjector> implements Depend
return new ViewContainerRef_(this._preBuiltObjects.viewManager, this.getElementRef());
}

getNestedView(): viewModule.AppView { return this._preBuiltObjects.nestedView; }
getNestedView(): AppView { return this._preBuiltObjects.nestedView; }

getView(): viewModule.AppView { return this._preBuiltObjects.view; }
getView(): AppView { return this._preBuiltObjects.view; }

directParent(): ElementInjector { return this._proto.distanceToParent < 2 ? this.parent : null; }

Expand Down Expand Up @@ -1044,13 +1044,13 @@ export class QueryRef {
}
}

private _visitViewContainer(vc: viewModule.AppViewContainer, aggregator: any[]) {
private _visitViewContainer(vc: AppViewContainer, aggregator: any[]) {
for (var j = 0; j < vc.views.length; j++) {
this._visitView(vc.views[j], aggregator);
}
}

private _visitView(view: viewModule.AppView, aggregator: any[]) {
private _visitView(view: AppView, aggregator: any[]) {
for (var i = view.elementOffset; i < view.elementOffset + view.ownBindersCount; i++) {
var inj = view.elementInjectors[i];
if (isBlank(inj)) continue;
Expand Down
52 changes: 38 additions & 14 deletions tools/broccoli/angular_builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@ type ProjectMap = {
[key: string]: boolean
};

type Options = {
projects: ProjectMap;
noTypeChecks: boolean;
generateEs6: boolean;
}
;

/**
* BroccoliBuilder facade for all of our build pipelines.
*/
Expand All @@ -25,20 +32,20 @@ export class AngularBuilder {
constructor(public options: AngularBuilderOptions) { this.outputPath = options.outputPath; }


public rebuildBrowserDevTree(projects: ProjectMap): Promise<BuildResult> {
this.browserDevBuilder = this.browserDevBuilder || this.makeBrowserDevBuilder(projects);
public rebuildBrowserDevTree(opts: Options): Promise<BuildResult> {
this.browserDevBuilder = this.browserDevBuilder || this.makeBrowserDevBuilder(opts);
return this.rebuild(this.browserDevBuilder, 'js.dev');
}


public rebuildBrowserProdTree(projects: ProjectMap): Promise<BuildResult> {
this.browserProdBuilder = this.browserProdBuilder || this.makeBrowserProdBuilder(projects);
public rebuildBrowserProdTree(opts: Options): Promise<BuildResult> {
this.browserProdBuilder = this.browserProdBuilder || this.makeBrowserProdBuilder(opts);
return this.rebuild(this.browserProdBuilder, 'js.prod');
}


public rebuildNodeTree(projects: ProjectMap): Promise<BuildResult> {
this.nodeBuilder = this.nodeBuilder || this.makeNodeBuilder(projects);
public rebuildNodeTree(opts: Options): Promise<BuildResult> {
this.nodeBuilder = this.nodeBuilder || this.makeNodeBuilder(opts.projects);
return this.rebuild(this.nodeBuilder, 'js.cjs');
}

Expand All @@ -58,16 +65,30 @@ export class AngularBuilder {
}


private makeBrowserDevBuilder(projects: ProjectMap): BroccoliBuilder {
let tree = makeBrowserTree({name: 'dev', typeAssertions: true, projects: projects},
path.join(this.outputPath, 'js', 'dev'));
private makeBrowserDevBuilder(opts: Options): BroccoliBuilder {
let tree = makeBrowserTree(
{
name: 'dev',
typeAssertions: true,
projects: opts.projects,
noTypeChecks: opts.noTypeChecks,
generateEs6: opts.generateEs6
},
path.join(this.outputPath, 'js', 'dev'));
return new broccoli.Builder(tree);
}


private makeBrowserProdBuilder(projects: ProjectMap): BroccoliBuilder {
let tree = makeBrowserTree({name: 'prod', typeAssertions: false, projects: projects},
path.join(this.outputPath, 'js', 'prod'));
private makeBrowserProdBuilder(opts: Options): BroccoliBuilder {
let tree = makeBrowserTree(
{
name: 'prod',
typeAssertions: false,
projects: opts.projects,
noTypeChecks: opts.noTypeChecks,
generateEs6: opts.generateEs6
},
path.join(this.outputPath, 'js', 'prod'));
return new broccoli.Builder(tree);
}

Expand Down Expand Up @@ -128,11 +149,14 @@ function broccoliNodeToBuildNode(broccoliNode) {

return new BuildNode(tree.description || tree.constructor.name,
tree.inputPath ? [tree.inputPath] : tree.inputPaths, tree.cachePath,
tree.outputPath, broccoliNode.subtrees.map(broccoliNodeToBuildNode));
tree.outputPath, broccoliNode.selfTime / (1000 * 1000 * 1000),
broccoliNode.totalTime / (1000 * 1000 * 1000),
broccoliNode.subtrees.map(broccoliNodeToBuildNode));
}


class BuildNode {
constructor(public pluginName: string, public inputPaths: string[], public cachePath: string,
public outputPath: string, public inputNodes: BroccoliNode[]) {}
public outputPath: string, public selfTime: number, public totalTime: number,
public inputNodes: BroccoliNode[]) {}
}
Loading
X Tutup