X Tutup
Skip to content

Optimize unit tests#5299

Closed
vsavkin wants to merge 1 commit intoangular:masterfrom
vsavkin:make_broccoli_great_again
Closed

Optimize unit tests#5299
vsavkin wants to merge 1 commit intoangular:masterfrom
vsavkin:make_broccoli_great_again

Conversation

@vsavkin
Copy link
Copy Markdown
Contributor

@vsavkin vsavkin commented Nov 15, 2015

Before: initial 20s incremental: 5.5s
Before with noEmitOnError=false manually patched-in: initial 18s incremental: 3.15s
After: initial 7.7s incremental: 0.5s

It is faster for the following two reasons:

  • no es6
  • no files from node_modules go into the browser tree.

Now, the time is split more-or-less equally between Funnel, MergeTrees, and TS.

This PR also fixed the circular dependencies check that was broken.

@vsavkin vsavkin added the action: review The PR is still awaiting reviews from at least one requested reviewer label Nov 15, 2015
@vsavkin vsavkin changed the title Make broccoli great again Optimize unit tests Nov 15, 2015
@vsavkin vsavkin force-pushed the make_broccoli_great_again branch 2 times, most recently from b48c98a to 437ceb1 Compare November 15, 2015 22:50
Copy link
Copy Markdown
Contributor

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

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

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

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

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.

@IgorMinar
Copy link
Copy Markdown
Contributor

discussed in person. lgtm

@IgorMinar IgorMinar added pr_state: LGTM action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Nov 16, 2015
@vsavkin vsavkin force-pushed the make_broccoli_great_again branch from 70a031c to a6f63b9 Compare November 16, 2015 23:47
@vsavkin vsavkin removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews action: discuss action: review The PR is still awaiting reviews from at least one requested reviewer labels Nov 16, 2015
Since editors and IDEs do typechecking and show errors in place,
often there is no benefit to running type checking in our test pipeline.
This PR allows you to disable type checking:

gulp test.unit.js --noTypeChecks

This commit also makes es6 generation optional.

fix(build): removes unnecessary circular dependencies
@mary-poppins
Copy link
Copy Markdown

Merging PR #5299 on behalf of @vsavkin to branch presubmit-vsavkin-pr-5299.

@angular-automatic-lock-bot
Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

X Tutup