Skip to content
This repository has been archived by the owner on Jul 29, 2024. It is now read-only.

fix(grep): change excluded tests to disabled instead of pending #4673

Merged
merged 2 commits into from
Mar 28, 2018

Conversation

eileenzheng
Copy link

This is an attempt to fix #3952 .

In Jasmine, beforeAll() is executed for pending tests, but not disabled tests. This pr set all tests that are excluded from running to disabled so the corresponding beforeAll() won't run.

@eileenzheng
Copy link
Author

Is there anything I can do to get this pr merged? @qiyigg

@qiyigg
Copy link
Contributor

qiyigg commented Feb 12, 2018

Hi, I have rebuilt the test, let's see that first. It seems the circleCI is not stable recently.
In general, I will take a look at the test first, if the test doesn't pass, I won't merge it.

@eileenzheng
Copy link
Author

@qiyigg OK thank you. I was a bit confused about circleCI as it fails in master as well...The unstableness explains it.

@raghulraj
Copy link

Any update on merging this fix?

@qiyigg
Copy link
Contributor

qiyigg commented Mar 6, 2018

It seems there's something wrong with the circle CI again.
I will take a look at the circle CI as well as this PR and ideally make a new release this week, sorry for the late response.

@marcdacz
Copy link

any updates on this issue? thanks heaps!

@qiyigg
Copy link
Contributor

qiyigg commented Mar 27, 2018

@eileenzheng
To make the CircleCI run properly, you probably need to rebase your PR to the head. I found the PR was created before my CircleCI fix was checked in.

One little concern is change filtered test to disabled will affect the result reporting. But it might be ok in this situation

Before:
151 specs, 0 failures, 104 pending specs
Finished in 15.257 seconds

After:
Ran 47 of 151 specs
47 specs, 0 failures
Finished in 14.705 seconds

@eileenzheng
Copy link
Author

@qiyigg thanks! I just pushed the rebase commit and all checks passed.

@qiyigg
Copy link
Contributor

qiyigg commented Mar 28, 2018

thanks, merged.

@qiyigg qiyigg merged commit c63b99e into angular:master Mar 28, 2018
@eileenzheng
Copy link
Author

thank you!!

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.

Using --grep will call every defined beforeAll() in your suite/spec
5 participants