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

Adds support for runInInjectionContext #664

Open
KryptonBD opened this issue Jul 9, 2024 · 3 comments
Open

Adds support for runInInjectionContext #664

KryptonBD opened this issue Jul 9, 2024 · 3 comments

Comments

@KryptonBD
Copy link

KryptonBD commented Jul 9, 2024

Description

Add support for TestBed.runInInjectionContext on spectator

Proposed solution

N/A

Alternatives considered

N/A

Do you want to create a pull request?

No

@emqi
Copy link
Contributor

emqi commented Jan 23, 2025

Hi @NetanelBasal,

First of all, I would like to thank you for creating this fantastic tool that Spectator is, it's been my go-to for writing tests in Angular for years. 💪

With the rise of popularity with DI functions in Angular, I've found myself missing out on runInjectionContext support in Spectator as well. I'm happy to try to contribute it myself, but first I'd like to propose & discuss my implementation idea as it deviates slightly from the existing Spectator APIs.

Proposed solution

The general support for runInInjectionContext is very easy to add:

export abstract class BaseSpectator {
  // ...
  public runInInjectionContext<T>(fn: () => T): T {
    return TestBed.runInInjectionContext(fn);
  }
}

Just like that, runInInjectionContext method should now be available on all Spectator instances! While pretty cool I don't think it's particularly useful in context of testing DI functions. Each of the existing factories requires some Angular building block to be also provided as the argument. Sure, we can use simple placeholders, but if we choose to do so, we might as well test our function as a class method.

It seems clear that something more is needed to make this change useful: a Spectator instance (let's call it SpectatorInjectionContext) that allows us to test a function without any unnecessary bloat. This is where my idea may turn out to be a bit controversial. For the new factory I'd narrow down the BaseSpectatorOptions to accept only 'mocks', 'mockProvider', 'providers' and 'imports'.

export type SpectatorInjectionContextOptions = Pick<
  BaseSpectatorOptions,
  "imports" | "mockProvider" | "mocks" | "providers"
>;

export function createInjectionContextFactory(
  options: SpectatorInjectionContextOptions
): SpectatorInjectionContextFactory {
  //...
}

The reason I suggest such type restriction is because I think it nicely highlights the expected use case for this particular Spectator instance. If that's too much of a limitation, BaseSpectatorOptions can be used as well. Regardless of the choice, there are only optional properties here so both types allows an empty object to be passed as an argument; this can be prevented with some TypeScript magic which I'm not including here for the sake of simplicity.

The new factory can then be used just like that:

describe("...", () => {
  // TestModule provides TestService
  const createContext = createInjectionContextFactory({
    imports: [TestModule],
    providers: [{ provide: TEST_TOKEN, useValue: "abcd" }],
  });

  let spectator: SpectatorInjectionContext;

  beforeEach(() => (spectator = createContext()));

  it("should execute fn in injection context", () => {
    const service = spectator.inject(TestService);
    service.flag = true;

    const result = spectator.runInInjectionContext(() => testFn(2));
    expect(result).toEqual({ token: "abcd", flag: true, arg: 2 });
  });
});

This gives us a nice Spectator-ish way of testing DI functions, but at this point it's hard to ignore the similarity to the pure TestBed counterpart:

it("should execute fn in injection context", () => {
  // TestModule provides TestService
  TestBed.configureTestingModule({
    imports: [TestModule],
    providers: [{ provide: TEST_TOKEN, useValue: "abcd" }],
  });

  const service = TestBed.inject(TestService);
  service.flag = true;

  const result = TestBed.runInInjectionContext(() => testFn(2));
  expect(result).toEqual({ token: "abcd", flag: true, arg: 2 });
});

It doesn't really feel like the Spectator makes our lives any simpler by enabling this support and overall makes me question whether this brings some actual value beyond just feature parity with TestBed. I'm happy to hear your thoughts on this and ready to follow-up with a Pull Request if such implementation is deemed worthwhile. 😀

Why make it different from other factories?

This is probably an obvious question that I prefered to leave for the end. Other Spectator factories universally employ the typeOrOptions pattern which allows us to either pass just the Angular class or an option object which combines BaseSpectatorOptions with a dedicated slot for that class. Why not apply this pattern here?

To keep the answer short - because DI functions are not classes. All we want to do in order to test them is setup some providers and run them in Injection Context. There's no need to pass the function internally to TestBed's configureTestingModule or to inherit its shape for future access. In the end we still need to explicitly pass our function as an argument to the runInInjectionContext and a single instance of SpectatorInjectionContext should allow us to test many different DI functions rather than just limit us to one.

@NetanelBasal
Copy link
Member

Hi, seems like a nice idea. I'd love to see a PR :)

@emqi
Copy link
Contributor

emqi commented Jan 24, 2025

It's up for review 🙂 #690

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

No branches or pull requests

3 participants