Can't spy SignalStore feature method called inside another feature method

IhorMaistrenko commented 4 weeks ago

Which @ngrx/* package(s) are the source of the bug?

signals

Minimal reproduction of the bug/regression with instructions

Create two custom store features with methods
Call method from one feature inside another feature method (i.e. method2 calls method1 from another feature)
Go to spec file where you test signal store and do next steps:
- Spy on method1
- call method2
- check that method1 was called (i.e. expect(method1).toHaveBeenCalled();)

Project to check reproduction: https://stackblitz.com/github/IhorMaistrenko/signal-store-feature-methods-testing

There are 2 tests: one is calling methods located at the same feature, second one is calling method inside method from another feature.

Expected behavior

Expect test to pass, because store contains method1 and method1 has been called inside method2.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s)

Test project was create on this environment: NgRx/signals: 18.1.1 Angular: 18.2.0 Node: v20.12.0 jasmin-core: 5.2.0 Karma: 6.4.0 OS: MacOS Browser: Chrome

Issue initially was found with this setup: NgRx/signals: 18.1.0 Angular: 18.2.4 Node: v22.3.0 Jest: 29.7.0 OS: Windows Browser: Chrome

Other information

Bug was reproduced in different projects with both Jasmine and Jest. Your Stackblitz project doesn't contain tests, I've tried to add them but got some errors. My project successfully runs in Stackblitz or you can clone if from Github. It is a blank project, only @ngrx/signals added.

P.S. I report bug for the first time. Please let me know if you have any questions

FYI @pawel-twardziak

I would be willing to submit a PR to fix this issue

[ ] Yes
[ ] No

pawel-twardziak commented 4 weeks ago

Hi @IhorMaistrenko

this case is very interesting one, cause the final store looks like a simple class instance with methods:

obraz

But hmm the methods do not come from the prototype, seems like they are attached to the class instance object that is already created.

markostanimirovic commented 4 weeks ago

The same problem can be reproduced without SignalStore:

it('fails', () => {
  function foo() {}
  function bar() {
    foo();
  }

  const myStore = { foo, bar };
  const spy = jest.spyOn(myStore, 'foo');

  myStore.bar();
  expect(spy).toHaveBeenCalled(); // error: `spy` is never called
});

it('fails (with SignalStore)', () => {
  const MyStore = signalStore(
    withMethods(() => {
      function foo() {}
      function bar() {
        foo();
      }

      return { foo, bar };
    })
  );

  const myStore = new MyStore();
  const spy = jest.spyOn(myStore, 'foo');

  myStore.bar();
  expect(spy).toHaveBeenCalled(); // error: `spy` is never called
});

I'd recommend against mocking methods within the same store to test other methods in that store. Here's why:

When we mock internal methods, we're testing implementation details rather than behavior.
These tests become high-maintenance: any internal refactoring requires updating both the implementation and test mocks.
A store and its methods should be considered a single unit. Mocking methods within it means we're creating test boundaries inside what should be treated as one unit.

Instead, I'd suggest testing the actual behavior and mocking external dependencies (services, other stores, etc.) rather than internal methods. If you find yourself wanting to mock internal methods, it might indicate the store has too many responsibilities and could benefit from being split into smaller, more focused stores.

pawel-twardziak commented 4 weeks ago

Hi @markostanimirovic :) Thank you for the explanation. One small doubt regarding the test examples:

In these cases it will fail because the function bar is calling the source function expression foo. And what we are spying in here is not the foo itself but its alias on the store. And indeed, this is how the store is being created internally (this !== store) - which is not intuitive for programmers imho.

Nevertheless, I am digging around the source code of the store and what I can see from there is that this object is not exactly the store itself.

Let me present how I would modify the store ecosystem factory functions. I will just raise a draft PR (nothing binding) just to showcase why the test does not work (regardless of what is being tested and any conceptual circumstances of the testing).

Myabe it is defind the way it is for some conceptual reason I don't know. So if am wrong, correct me please when you see my draft PR.

pawel-twardziak commented 3 weeks ago

Hi @markostanimirovic and @IhorMaistrenko sorry for for the dalay but we have an extended holiday weekend now in Poland :) Btw @markostanimirovic your contrubution to ngrx in terms of signals is amazing one 💯 It brings a huge value to Angular signals world 🤟 Thank you! 💟 🥇 I vote for you for the @NgPoland award this year 💯

@IhorMaistrenko and I we are working together btw 👍 I have some thought on the source code I would love to share with you @markostanimirovic in regard to:

how to properly test the signal store
quite sad issue related to a signal store instance type which is not being inferred correctly in Angular component template 😢 (from my perspective it is a result of complex typing of the store, might be wrong)

Anyway, will raise my showcase dratf PR tomorrow ;) Today and tomorrow will spend some time on it.

Have a great weekend guys!

IhorMaistrenko commented 3 weeks ago

@markostanimirovic thank you for explanation, now it makes more sense. When I saw this issue, I did exactly what you suggested: spied on service called in method1 and it worked.

In real world project I have custom feature that has method responsible for displaying notifications. And this feature will be added to other stores for the same purpose. My idea was not mocking it, but make sure that method was called. Custom store feature is a building block of store, so I thought it is ok to expect that I can spy for methods in my store if I have added this feature.

rainerhahnekamp commented 3 weeks ago

@IhorMaistrenko if you say your feature displays notifications, then it is probably using some services. For example in Angular Material you have MatSnackBar for that. Why don't you mock that service instead a method of a SignalStore?

pawel-twardziak commented 3 weeks ago

Hi @rainerhahnekamp and @markostanimirovic yes, we can mock external services. This is feasible. But it would be way more intuitive if we could just spy on an exact method on the store, what we normally do with regular services.

But it is not applicable to the store service. It is because each store feature gets its own store snapshot injected as an argument with method/signa/computed references (to the sources) in it. And the final public store instance gets its own references too what causes the behaving we are facing. This means when spy on any method/signal/computed on the final public store instance, we do that only for the final public store instance - none of the snapshots gets the same method/signal/computed reference spied on.

Is this for some reason that each of the snapshots and the final store are all independent that way?

I think the final store could get all the methods/signals/computed hoisted/moved/attached to it and then the store features would have called with a specific final store snapshot injected at that point and the snapshot would have specific getters return the methods/signals/computed from the final store. I strongly believe that way of making the composition would be more straightforward and simple.

This is what I wanna showcase by my draft PR.

Does it add up?

IhorMaistrenko commented 3 weeks ago

@rainerhahnekamp yes, I already did this and it works. Thank you.

pawel-twardziak commented 3 weeks ago

Ok, I see that my proposal is not met with interest, I give up. Thaks for your explanation guys regarding the issue 👍

rainerhahnekamp commented 3 weeks ago

Hi @pawel-twardziak, to be honest I didn't even find the time to think your proposal, through. But my first impression was to change some parts of the internal, which - honestly speaking - will have a low chance. Especially if we consider its outcome.

You can always try to come up with a PR and we can take a look at it. But as I said, I'm not sure that you have a guarantee.

pawel-twardziak commented 3 weeks ago

Hi @rainerhahnekamp thank you for dropping some lines in response to my comments :) Yeah, it is clear and coherent you have no time in the recent and upcaming days :) There are the NG/JS-Poland conferrences and workshops and you are gonna run both the session and the workshop :) Good luck and see you there!

Ok, makes sense. Will raise a PR but it will probably happen in around two weeks. Maybe it will add up.

👋

ngrx / platform