-
Notifications
You must be signed in to change notification settings - Fork 522
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
fix(jasmine): transitive specs are no longer added to the test suite #2576
fix(jasmine): transitive specs are no longer added to the test suite #2576
Conversation
@@ -25,7 +25,7 @@ def _js_sources_impl(ctx): | |||
depsets = [] | |||
for src in ctx.attr.srcs: | |||
if JSModuleInfo in src: | |||
depsets.append(src[JSModuleInfo].sources) | |||
depsets.append(src[JSModuleInfo].direct_sources) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
technically this is a breaking change, since users who were relying on nested packages having their tests run will now just get nothing running those tests. I think maybe we have to hold this change for a 4.x branch, unless we want to add some opt-in flag for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless we want to add some opt-in flag for it.
I would prefer this option. Our tests are broken at the moment, so it would be great to get this fix faster than waiting for 4.x
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the fix a bit:
- Added
use_direct_specs
opt-in flag. - With this flag, only specs from direct sources of
srcs
will be added to execution. Specs fromdeps
no longer taken into account.
after some thought I agree the old behavior is a bug |
4706eb3
to
5249698
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks!!
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Jasmine runner adds all transitive specs to active execution, which results in some specs running multiple times.
This can be especially problematic when there are multiple
ts_projects
with test specs that depend on each other. For example:Issue Number: N/A
What is the new behavior?
Only specs from direct deps are added to active execution
Does this PR introduce a breaking change?
Other information