Skip to content
This repository was archived by the owner on Apr 4, 2025. It is now read-only.

fix(@schematics/angular): update application and component spec schem… #224

Closed
wants to merge 1 commit into from
Closed

fix(@schematics/angular): update application and component spec schem… #224

wants to merge 1 commit into from

Conversation

delasteve
Copy link
Contributor

…atics

@trimeyko
Copy link

Async should be used, so there is #233 request to fix #182

@delasteve
Copy link
Contributor Author

@trimeyko, async() is only necessary when your function contains a promise or doing some other asynchronous activity. It is not necessary to put it on every it() test method. The only call that needs to have it is the beforeEach which has a promise in it (compileComponents()). Therefore, you only need it there.

@trimeyko
Copy link

@delasteve just found original pull in ng-cli where async were added ( 48a695e ), yes that probably an accident. Currently I'm using asyncs since lot of component tests without them leads karma to await chrome for about 5-6 minutes ( with current cli configuration you will get disconnect ), I'm don't want to say it's the best way to solve this, but as a workaround for endusers.

@delasteve
Copy link
Contributor Author

@trimeyko, I'll leave it to @filipesilva to clarify his intentions for adding it. Adding an async() to each of the it() blocks doesn't do anything for the code to run.

Don't get me wrong, there are use cases for async() on it blocks. My suggestion, without looking at your code, would be to try to deduce which tests actually need the wrapper and only use async() there. You'll, hopefully, find the vast majority probably do not need it. I have a project right now that runs roughly 1000 tests in about 30 seconds, and only a handful of them use async or fakeAsync.

IMO we shouldn't start users believing that async() is necessary to run tests, because in most cases, it's not. If they need more advanced features (eg: async(), fakeAsync(), tick()) then there is plenty of documentation on angular.io and other websites about it.

@filipesilva
Copy link
Contributor

@Brocco can you chime in on this topic?

@Brocco
Copy link
Contributor

Brocco commented Oct 31, 2017

What is the issue that prompted this PR?

@delasteve
Copy link
Contributor Author

delasteve commented Oct 31, 2017

@Brocco, it was just general schematic clean up. Whitespace, import ordering, and aligning AppComponent to be closer to the Component schematic (e.g.: dropping async() on its).

For example, back when we had blueprints, it looked something like this (random commit before the move to devkit on https://github/angular/angular-cli showing this file):

https://github.com/angular/angular-cli/blob/55b9d5ca0c7b8f7083eeab555025ecc4d6dd301f/packages/%40angular/cli/blueprints/ng/files/__path__/app/app.component.spec.ts

@alexeagle
Copy link
Contributor

Sorry @delasteve - this got lost when we moved back to the angular/angular-cli repo. If you still think this pull request is relevant, could you please re-base on that repo and open a new PR there? Thanks and sorry for the extra churn.

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.

6 participants