Unexpected behavior for providedIn services in TestBed

[shairez]

providedIn has brought a lot of goodies into the Angular world, especially for lazy loaded modules configurations.

BUT...

On the testing side, it's current behavior is confusing and unwanted for isolation.

I'm submitting a...

[x] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question
[ ] Other... Please describe:

Current behavior

providedIn providers are being loaded automatically by TestBed 's Testing Module.

Expected behavior

I test all of my units in isolation.

Whilte testing, I would like the providers to not be configured automatically so I could see an error if I forgot to stub / mock a certain provider.

If we're not going to change this behavior, at least let's add a FLAG that gives people the opportunity to configure TestBed to fail when we're not manually configuring providedIn services.

Minimal reproduction of the problem with instructions

look at the following project:

It's not clear from the hello.component.spec.ts that an ajax request somewhere down the injection tree is being called

open the console and see the message from the product.service.ts

https://stackblitz.com/edit/angular-testing-providedin-example

What is the motivation / use case for changing the behavior?

Imagine the following (very realistic) scenario:

1. You write an test for your main component (isolating all of it's immediate dependencies)

2. This component has child components

3. Sometime in the future, another developer adds a dependency to one of the child components which has some side effect (ajax request or an error, or a change to the router, etc)

4. It will take you quite a while ( in a large scale app) to track down the specific reason

The bad part about this is that it's very sneaky and not obvious because the failure usually will come later and not as soon as possbie.

So I suggest, to ignore providedIn services in TestBed on purpose (or at least add a FLAG that allows you to achieve this behavior for the sake of our test debugging time)

Environment

Angular version: 6.1.2


Browser:
- [x] Chrome (desktop) version XX
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [ ] Edge version XX
 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

[FDIM]

I'd like to tackle this issue and ensure that testing module is always isolated / empty. I'd say that this behavior should not change / services provided in root should fail to resolve during tests.

EDIT: Managed to reproduce the issue by adding additional test in test_bed specs. Currently tracked down where provider is created and looking for a way to change the behavior in test. Possibly by overwriting provider def and removing providedIn option / or checking that early in custom test injector.

[JohannesHoppe]

I completely agree with @shairez. We have an issue, we have a solution (PR). Are there any chances that the current behavior will be fixed? Or did we already accepted integration tests as the new default?! 🤨

[FDIM]

I hope not! Sadly this issue is not an issue for google internally (not affiliated) and thus has very little priority - At least this is my general experience when trying to contribute. Maybe 2019 will be a good year.

[niklas-wortmann]

I do agree with @shairez too, the problem with the proposed solution of #26088 that it would break a lot of tests (at least from my perspective and for a lot of projects I've seen) and therefore it should be considered a breaking change. Also in the pr it is correctly stated out that this kind of behavior is quite nice for integration tests. To advance with this in a non-breaking way I would rather create a new method in the TestBed, e.g. TestBed.configureUnitTestingModule.

[andreea-s]

The behavior is nice for integration tests, but not so much for unit tests. It sort of replicates the angularjs issue where one would load a module and all the services in there would be automatically provided. Having a clean environment where one has to explicitly provide each service is more practical, than trying to figure out what the dependency graph is and what services have not yet been provided in the tests.

[christophamma]

We are experiencing this issue as well in a new rather large angular project. We have a lot of services that depend on other services and unless you check all dependencies manually and include them in the TestBed, you end up with integration tests instead of unittests without even noticing. It is almost certain that developers forget to mock dependencies. I'm really surprised that this done on purpose as it seems like a major flaw to me.

Does anyone have a workaround for this or is there any way to check for non-mocked dependencies in an existing code-base?

[FDIM]

@christophamma sadly only one, skip using provided in root :/ This is what we ended up agreeing to do in the project I'm in. Tree shaking dead services in a project is not as important as in a library IMO.

[christophamma]

@christophamma sadly only one, skip using provided in root :/ This is what we ended up agreeing to do in the project I'm in. Tree shaking dead services in a project is not as important as in a library IMO.

@FDIM yes, seems better than ending up with flaky unittests. Do you simply agree on it or is there a way to enforce it in angular?

[FDIM]

It was enough to agree for us, on the other hand I'm pretty sure writing eslint rule would be fairly simple to enforce this.

[JamesHenry]

UPDATE TO REMOVE ALL DOUBT:

This will not be integrated into angular-eslint because it is recommending something which goes against the current recommendations of the Angular Team.

This is why I closed the issue requesting this on angular-eslint.

The below is merely intended as helpful guidance for folks who end up wanting to enforce things in their own codebases and incorrectly believe they need to have dedicated rules for forbidding certain syntax.

---

Creator of typescript-eslint and angular-eslint here 👋

This was flagged up to me via a request to create a dedicated rule for this case. I am hesitant to do that for something which is still evolving as a topic and does not yet have a dedicated solution from the Angular Team.

However, you do not necessarily need to create a dedicated lint rule for this, the built-in ESLint no-restricted-syntax rule could be used to achieve the desired result:

{
  "rules": {
    "no-restricted-syntax": [
      "error",
      {
        "selector": "Decorator[expression.callee.name=Injectable] Property[key.name=providedIn][value.value=root]",
        "message": "Do not use `providedIn: root` in Injectables"
      }
    ]
  }
}

Example of it in action:

https://typescript-eslint.io/play/#ts=4.5.2&sourceType=module&showAST=es&code=JYWwDg9gTgLgBAbzgSQHYCsCmBjGBDAIwBtM4BfOAMyghDgHIABPVAcwFci8oB6baTPQDcAKBGM0WXIRIAKBCLhK4YGgDdgAE0ya0ALgY0IMeiLIBKEdi4BnG3ABiECIjIigA&rules=N4Igdg9gtATgpgZwC4wJYGMlwCZQQTzCQEMAPEALgG0Q4YYIYQAaUBOAGzk0cpABFujYkkZU4pAA7wECVBDAA6dMQ5c4isMQC2cALwBJMACtuJAEZcAugAIbABQaS6SfFQDWcfJp37pEADdUbBwjKyoA1QBXDUiOGL0GCCQrFhBdWWIAczg+fggbSCQbKPYbAAN-IJDsIwobJKRym1QwGyNTTGJLRBAAXys+oA&tsConfig=N4IgpgHgDmBOCWBbMA7ALgQwDYBEwGMB7WDNYgZxAC41YBXMAGnEXjTyJLNgFkxMAJqQzVaDAL5A

[JohannesHoppe]

@JamesHenry @coyoteecd I'm confused. Injectable({ providedIn: 'root' }) is the correct way to use Injectables. this rule would effect the old, no longer recommended syntax (without provideIn). In the past, services were not tree-shakable and that was not good at all!

The problem is that the unit tests are implemented in an unpleasant way. But this cannot mean that we weaken the productive code!

[JamesHenry]

@JohannesHoppe hence why I am unwilling to implement this as a rule in angular-eslint

[JohannesHoppe]

Yes, this rule would recommend an anti-pattern! ☹️
I suggest to not include it in any official release.

[coyoteecd]

@JohannesHoppe as the other commenters above me, I have tried the "recommended way" and ended up wasting many hours debugging flaky tests in our test suites, because it's very easy to end up with a service automatically provided in your TestBed without you being aware of it.

The Angular team doesn't want to fix this - even though people have explained that it leads to unit test issues - calling it "by design". So we ended up doing what @FDIM is doing, stop using providedIn across the project. The sample no-restricted-syntax rule works fine for enforcing this and I'm glad that it's documented here for anyone else that hits this issue.

[christophamma]

@JamesHenry thanks for providing the example. We ended up writing our own rule as @FDIM suggested but this lead to issues in webstorm. The no-restricted-syntax version is much better.

I still think, that this should be solved, as IMHO it violates principles of good software design to implicitly inject real services within the TestBed.

[FDIM]

@JamesHenry @coyoteecd I'm confused. Injectable({ providedIn: 'root' }) is the correct way to use Injectables. this rule would effect the old, no longer recommended syntax (without provideIn). In the past, services were not tree-shakable and that was not good at all!

The problem is that the unit tests are implemented in an unpleasant way. But this cannot mean that we weaken the productive code!

Tree shaking is a very weak argument in application code - there should be no unused code IMO. Hence I'm advocating of not using it for easier maintenance.

For libraries it does solve a real problem, there is a higher chance of having shared code that is not used in all the projects.

[alxhub]

Out of curiosity, how would you propose handling this case:

import {UiCmp} from 'some/lib';
it('tests a component that uses a third party dependency', () => {
  @Component({template: '<ui-cmp', ...})
  class TestCmp {}

  TestBed.configureTestingModule({disableRootProviders: true});

  const fix = TestBed.createComponent(TestCmp);
  fix.detectChanges();

  expect(fix.nativeElement.innerHTML).toEqual('...');
}

if UiCmp requires a private (not exported) providedIn: 'root' service to function?

[coyoteecd]

@alxhub quick answer would be "let it fail". But yeah, you're having a point there, there will be many corner cases. Another one is that I wouldn't want to explicitly provide NgZone, ElementRef etc. in each and every test.

Maybe a different idea that provides a sufficient level of safety: is there any way to detect at runtime that a service is being injected in the TestBed? An assert in the constructor of our API services that they're not running in a test environment would work, and circumvent the issue with NgZone or other private services used by 3rd party components. Or maybe a decorator, or an option in the existing Injectable decorator, that marks an individual service as "not provided in test bed"...

[JeanMeche]

I'll reshare Alex' rationale about TestBed :

After some additional discussion on the team, we've decided to close this issue as something we don't plan on supporting.

Angular faces a tradeoff here. We absolutely understand and value the simplicity and power of a DI-based framework, where behavior is always abstracted through interfaces, and any part of the framework can be replaced or faked. The power of this kind of architecture is clearly demonstrated in other technologies (e.g. the Spring Framework).

On the other hand, we're responsible for updating thousands of Angular applications across Google's codebase, so we have a front row seat to the challenges developers face in maintaining their own applications. By far, the biggest friction we face in applying these updates is when applications have tests which mock framework features. At best, this causes these tests to diverge from reality when the behavior of the underlying feature changes. At worst, we're blocked from updating at all until we correct the tests (and users externally who are updating Angular would be similarly challenged).

As a result, we've become very conservative about giving tests the ability to mock framework behavior. Our message has been that code which depends on framework functionality should be using TestBed for testing, as that helps guarantee that the tests will match Angular's production behavior, and evolve with it over time.

Occasionally developers wish to write tests at a very small granularity, without a dependency on the framework. Our stance is that developers should then provide their own abstraction layers. This could involve extracting business logic out of components, so it has no dependency on Angular features like inputs, outputs, queries, templates, etc. Operations like effects could be moved behind service APIs that can be mocked/faked as needed.

A separate complaint that sometimes gets brought up is that "TestBed is too slow". Often when I've heard this, it's been based on historical experiences with TestBed that may no longer be valid. Prior to Ivy, TestBed performed JIT recompilation of all components in the test, on every test. This was indeed very slow. Ivy made TestBed something like 90% faster by reusing the compilation output between tests as much as possible. Our own framework tests went from taking several minutes to 10s of seconds.

There may still be cases where TestBed is slower than hand-written vanilla tests. This is sometimes because TestBed ensures that production behavior is replicated in full (e.g. destroying services at the end of the test). If you have use cases where you feel TestBed should be faster, please feel free to open an issue with a reproduction and we'd be happy to see if there are opportunities for optimization.

In the case we're discussing here. TestBed is expected to mimic the production behavior. In the case we're discussing here, if you use providedIn: 'root' you should expect the token to be provided by default.

If you do not want to provide it by default, you should remove providedIn:'root' and define the provider yourself.

[angular-automatic-lock-bot]

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.}