Skip to content
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

Issue in mocking service provided in directive in submodule #623

Closed
DavideCanton opened this issue May 30, 2021 · 12 comments · Fixed by #727
Closed

Issue in mocking service provided in directive in submodule #623

DavideCanton opened this issue May 30, 2021 · 12 comments · Fixed by #727

Comments

@DavideCanton
Copy link

Hi,

I'm trying to unit test a component (TodoComponent), injecting a service (ContextService). The service comes from the providers of a directive (ContextDirective) instantiating the component dynamically and providing the service as a "context".

The directive is declared and exported from module SharedModule and provides the service:

@Directive({
  selector: '[appContext]',
  providers: [ContextService]
})
export class ContextDirective implements OnInit
{
  // directive code
}

The component is declared in the TodoListModule, which imports the SharedModule, and injects the ContextService, which is a private instance for the component decorated with the directive:

@Component({
  selector: 'app-todo',
  template: `
    <span class="id">{{ todo.id }}.</span>
    <app-description [done]="todo.done" [description]="todo.description"></app-description>
    <input type="checkbox" [(ngModel)]="todo.done" [ngModelOptions]="{ standalone: true }" />
    <button (click)="remove()">Remove</button>
  `,
  styleUrls: ['./todo.component.scss']
})
export class TodoComponent
{
  constructor(private context: ContextService) { }
  // component code
}

The setup I'm using for testing the component is the following (I'm using spectator, but I already tried without it and the outcome is the same):

describe('TodoComponent', () =>
{
  let spectator: Spectator<TodoComponent>;
  let component: TodoComponent;
  let ctx: SpyObject<ContextService>;


  const module = MockBuilder(TodoComponent, TodoModule)
    .keep(FormsModule)
    .mock(SharedModule)
    .build();

  const factory = createComponentFactory(m(
    module,
    {
      component: TodoComponent,
      detectChanges: false
    }
  ));

  beforeEach(() =>
  {
    spectator = factory();
    component = spectator.component;
    ctx = spectator.inject(ContextService);
  });

  // actual tests

I get the error NullInjectorError: No provider for ContextService!, because it seems that the MockBuilder is not mocking the service.

I tried also adding .mock(ContextService), with imho should be sufficient because the component shouldn't know who is the provider of the service, it just needs an instance, but the error persists.

If I remove the .mock(SharedModule) and I mock explicitly the ContextService it works, but I get a lot of warnings because a subcomponent (DescriptionComponent) defined in SharedModule is not mocked, and I would like that it and all the other dependencies from SharedModule are mocked.

I tried debugging the issue, and it seems that the .mock(ContextService) line is ignored because the check in https://github.com/ike18t/ng-mocks/blob/master/libs/ng-mocks/src/lib/mock-builder/promise/add-missed-mock-declarations-and-modules.ts#L14 aborts the mocking process for ContextService because it is already present in the ngMocksUniverse.touches Set.

Here (https://github.com/DavideCanton/ng-mocks-issue) there is the repo for reproducing the issue, the tests not working are the ones for the TodoComponent, and they are already marked with fdescribe, so a npm run test is sufficient to reproduce the issue.

Am I missing something in configuring the MockBuilder?

@satanTime
Copy link
Member

Hi there,

thanks for the report. I'll take a look next days at the issue.

@satanTime
Copy link
Member

satanTime commented Jun 1, 2021

Hi @DavideCanton,

I think the issue is in the next circumstances:

  • ContextService exists in elements with ContextDirective
  • ContextDirective is present on elements with an input appContext
  • Nothing is happening, because createComponentFactory doesn't bind the input

Therefore, the NullInjectorError failure is correct.

If we consider the circumstances, we can make it work like that:

  • Render a temple with the input like <app-todo [appContext]="null"></app-todo>
  • extract ContextService from its debugNode
  • perform the test
import {MockBuilder, MockRenderFactory, ngMocks} from 'ng-mocks';
import { TodoModule } from 'app/todo/todo.module';
import { FormsModule } from '@angular/forms';
import { fakeAsync, tick } from '@angular/core/testing';
import { TodoComponent } from 'app/todo/todo/todo.component';
import { ContextService } from 'app/shared/services/context.service';
import { SharedModule } from 'app/shared/shared.module';

fdescribe('TodoComponent', () =>
{
  ngMocks.faster();

  beforeAll(() => MockBuilder(TodoComponent, TodoModule)
    .keep(FormsModule)
    .mock(SharedModule));

  let factory: MockRenderFactory<never>;
  beforeAll(() => factory = MockRenderFactory(`<app-todo [appContext]="null"></app-todo>`));

  fit('should create', fakeAsync(() =>
  {
    const fixture = factory(null, false);
    const ctx = ngMocks.get(fixture.point, ContextService);
    ctx.todo = { id: 1, description: 'aaa', done: false };
    fixture.detectChanges();

    expect(fixture.point.componentInstance).toBeTruthy();
  }));
});

However, in my option, this test looks more like an integration test between TodoComponent and ContextDirective.
Because of that I would add ContextDirective to .keep and would test it via its input instead of extracting ContextService.

import {MockBuilder, MockRenderFactory, ngMocks} from 'ng-mocks';
import {TodoModule} from 'app/todo/todo.module';
import {FormsModule} from '@angular/forms';
import {TodoComponent} from 'app/todo/todo/todo.component';
import {SharedModule} from 'app/shared/shared.module';
import {ContextDirective} from 'app/shared/directives/context.directive';

fdescribe('TodoComponent', () =>
{
  ngMocks.faster();

  beforeAll(() => MockBuilder(TodoComponent, TodoModule)
    .keep(FormsModule)
    .keep(ContextDirective)
    .mock(SharedModule));

  let factory: MockRenderFactory<never>;
  beforeAll(() => factory = MockRenderFactory(`<app-todo [appContext]="todo"></app-todo>`));

  fit('should create', () =>
  {
    const fixture = factory({
      todo: { id: 1, description: 'aaa', done: false },
    });

    expect(fixture.point.componentInstance).toBeTruthy();
  });
});

@DavideCanton
Copy link
Author

Hi, sorry for the late answer.

I don't agree with the premise of your analysis, because I'm not relying in my TodoComponent on the directive applied on it, the dependency on ContextService could come from any ancestor of TodoComponent, and still the issue would be present.

In my opinion ContextService is a dependency of the component and it should be provided to it (via mock or via provider) without any knowledge of who provides it.

@satanTime
Copy link
Member

Hi there,

I think, the best way to understand the problem is to try to implement this test in pure angular and TestBed without any testing library. It will have the same result, because ContextService is a local provider which exists only in contexts of ContextDirective and it cannot be injected outside of it.

The thing here is about how DI works. If nobody provided ContextService then TodoComponent would fail at runtime. That's why ng-mocks respects all imports and declarations. That allows to see failures about missing dependencies during testing, instead of getting 100% success on unit tests and then a failure at runtime.

If you don't want to care who provides ContextService, you could simply provide it via MockBuilder.provide:

  const module = MockBuilder(TodoComponent, TodoModule)
    .keep(FormsModule)
    .mock(SharedModule)
    .provide(MockProvider(ContextService))
    .build();

Then it can be accessed in a global scope outside of ContextDirective.

@DavideCanton
Copy link
Author

I see no problems in testing with TestBed:

import { async, ComponentFixture, fakeAsync, TestBed, tick } from '@angular/core/testing';
import { FormsModule } from '@angular/forms';
import { By } from '@angular/platform-browser';
import { DescriptionComponent } from 'app/shared/description/description.component';
import { ContextService } from 'app/shared/services/context.service';
import { TodoComponent } from 'app/todo/todo/todo.component';
import { CommonModule } from '@angular/common';
import { Spectator } from '@ngneat/spectator';

fdescribe('TodoComponent', () =>
{
  let component: TodoComponent;
  let fixture: ComponentFixture<TodoComponent>;
  let ctx: ContextService;

  beforeEach(async(() =>
  {
    TestBed.configureTestingModule({
      declarations: [TodoComponent, DescriptionComponent],
      imports: [
        FormsModule
      ],
      providers: [
        {
          provide: ContextService,
          useValue: jasmine.createSpyObj(ContextService, ['remove'])
        }
      ]
    })
      .compileComponents();
  }));

  beforeEach(() =>
  {
    fixture = TestBed.createComponent(TodoComponent);
    component = fixture.componentInstance;
    ctx = TestBed.inject(ContextService);
  });

  it('should create', fakeAsync(() =>
  {
    ctx.todo = { id: 1, description: 'aaa', done: false };
    fixture.detectChanges();
    tick();
    expect(component).toBeTruthy();
  }));

  it('should display item correctly if not done', fakeAsync(() =>
  {
    ctx.todo = { id: 1, description: 'aaa', done: false };
    fixture.detectChanges();
    tick();

    expect(fixture.debugElement.query(By.css('.id')).nativeElement.innerHTML).toBe('1.');
    expect(fixture.debugElement.query(By.css('input')).nativeElement.checked).toBe(false);
  }));

  it('should display item correctly if done', fakeAsync(() =>
  {
    ctx.todo = { id: 2, description: 'bbb', done: true };
    fixture.detectChanges();
    tick();

    expect(fixture.debugElement.query(By.css('.id')).nativeElement.innerHTML).toBe('2.');
    expect(fixture.debugElement.query(By.css('input')).nativeElement.checked).toBe(true);
  }));

  it('should update todo if checkbox clicked', fakeAsync(() =>
  {
    ctx.todo = { id: 2, description: 'bbb', done: false };
    fixture.detectChanges();
    tick();

    const input = fixture.debugElement.query(By.css('input'));
    input.nativeElement.click();

    fixture.detectChanges();
    tick();

    expect(fixture.debugElement.query(By.css('input')).nativeElement.checked).toBe(true);
    expect(ctx.todo.done).toBe(true);
  }));

  it('should emit if remove clicked', fakeAsync(() =>
  {
    ctx.todo = { id: 2, description: 'bbb', done: false };

    fixture.detectChanges();
    tick();

    const button = fixture.debugElement.query(By.css('button'));
    button.nativeElement.click();

    fixture.detectChanges();
    tick();

    expect(ctx.remove).toHaveBeenCalledTimes(1);
  }));
});

This is because I can provide a mock to ContextService without problems if I don't use ng-mocks.
I simply don't get why I should use .provide(MockProvider(ContextService)) instead of .mock(ContextService)

@satanTime
Copy link
Member

satanTime commented Jun 11, 2021

Ha, in this example, instead of mocking ContextService at the place of its declaration, you provide its instance in the global context. Now the test won't fail if the dependency is missing in app modules, what might be a potential disadvantage of this test.

If it is the original idea, then you should do the same in the original test and simply use .provide instead of .mock. And with .provide you can provide a mock to ContextService without problems also if you used ng-mocks.

Mocking and providing are different actions. The first one turns providers into mocks at places of their definition, whereas the second one adds them in the global context despite their place of definition.

@satanTime
Copy link
Member

What I think is that it might be a problem with export: true for such a local provider and it would make a sense to cover it, so

MockBuilder(TodoComponent, [TodoModule, ContextService])

would add ContextService to global providers because there is no way to export it.

@DavideCanton
Copy link
Author

Ok, then maybe I misunderstood the function of the .mock method.

@satanTime
Copy link
Member

satanTime commented Jun 11, 2021

Maybe, I think it can be a bit confusing and not fully clear.

What .mock does from the very first implementation - it mocks things at their definition.
However, if the thing hasn't been met during mocking process (not imported / not provided), then .mock adds it to the global context in imports, declarations or providers based on its type.

In my option, this creates an ambiguity: if ContextService has been provided somewhere, like in your example, then it has been mocked there, and therefore isn't accessible in the global context.
However, if it isn't provided at all (SharedModule isn't imported, specified), then it would be independently mocked and added to the global context via providers.

What I plan to do with v13 is to deprecate this behavior and consider all .mock and .keep calls as dependency: true.
Then using .mock(SharedModule) wouldn't add SharedModule to global imports unless it has been imported in TodoModule or configured like MockBuilder(TodoComponent, [TodoModule, SharedModule]).

But this all should be properly rethought and defined.

Sorry for inconvenience, I hope .provide covers your needs in full.
Also let's keep the ticket open, I'll update documentation with your case and try to solve export: true case for such a case.

@satanTime
Copy link
Member

satanTime commented Jun 19, 2021

Hi @DavideCanton,

I hope you are doing great.

I've added a fix for such a case. After the release, you could use MockBuilder like that:

const module = MockBuilder(
  [TodoComponent, FormsModule], // things to keep
  [TodoModule, SharedModule, ContextService], // things to mock and export
  // the exported things are guaranteed to be available form any context.
).build();

or with export flag:

  const module = MockBuilder(TodoComponent, TodoModule)
    .keep(FormsModule)
    .mock(SharedModule)
    .mock(ContextService, ContextService, {export: true})
    .build();

satanTime added a commit that referenced this issue Jun 19, 2021
fix(mock-builder): provides globally exported providers from directives and components #623
@satanTime
Copy link
Member

v12.1.1 has been released and contains a fix for the issue. Feel free to reopen the issue or to submit a new one if you meet any problems.

@DavideCanton
Copy link
Author

Thanks @satanTime , I'll try it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants