-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Can't spy SignalStore feature method called inside another feature method #4577
Comments
this case is very interesting one, cause the final store looks like a simple class instance with methods: But hmm the methods do not come from the prototype, seems like they are attached to the class instance object that is already created. |
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:
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. |
Hi @markostanimirovic :) Thank you for the explanation. One small doubt regarding the test examples: In these cases it will fail because the function Nevertheless, I am digging around the source code of the store and what I can see from there is that 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. |
Hi @markostanimirovic and @IhorMaistrenko sorry for for the dalay but we have an extended holiday weekend now in Poland :) @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:
Anyway, will raise my showcase dratf PR tomorrow ;) Today and tomorrow will spend some time on it. Have a great weekend guys! |
@markostanimirovic thank you for explanation, now it makes more sense. 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. |
@IhorMaistrenko if you say your feature displays notifications, then it is probably using some services. For example in Angular Material you have |
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. 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. This is what I wanna showcase by my draft PR. Does it add up? |
@rainerhahnekamp yes, I already did this and it works. Thank you. |
Ok, I see that my proposal is not met with interest, I give up. Thaks for your explanation guys regarding the issue 👍 |
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. |
Hi @rainerhahnekamp thank you for dropping some lines in response to my comments :) Ok, makes sense. Will raise a PR but it will probably happen in around two weeks. Maybe it will add up. 👋 |
Which @ngrx/* package(s) are the source of the bug?
signals
Minimal reproduction of the bug/regression with instructions
method2
callsmethod1
from another feature)method1
method2
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
andmethod1
has been called insidemethod2
.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
The text was updated successfully, but these errors were encountered: