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

docs(signals): add testing guide #4461

Merged
merged 15 commits into from
Sep 17, 2024

Conversation

rainerhahnekamp
Copy link
Contributor

@rainerhahnekamp rainerhahnekamp commented Jul 26, 2024

PR Checklist

The formatted version is available at: https://github.com/rainerhahnekamp/ngrx/blob/docs/signals/testing/projects/ngrx.io/content/guide/signals/signal-store/testing.md.

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[x] Documentation content changes
[ ] Other... Please describe:

Closes #4206

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Copy link

netlify bot commented Jul 26, 2024

Deploy Preview for ngrx-io ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit df2912e
🔍 Latest deploy log https://app.netlify.com/sites/ngrx-io/deploys/66e89c8cd7b55300083f5d11
😎 Deploy Preview https://deploy-preview-4461--ngrx-io.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@rainerhahnekamp rainerhahnekamp marked this pull request as ready for review August 1, 2024 12:18
@jdegand
Copy link

jdegand commented Aug 1, 2024

Missing backtick on line 56 around TestBed. Some inconsistency in using TestBed and TestBed. Line 62 is missing the backticks.

Could add the links to ng-mocks and other libraries mentioned.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some thoughts on the structure of the page.
When the markdown is converted to a document page I'll go over it again and spent more time on style/grammar.


### On Testing in General

A Signal Store is a simple Angular Service, so the same testing techniques you use for services apply to Signal Stores as well. This guide focuses on providing examples for common testing scenarios.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two minor suggestions to ensure consistency with the rest of the @ngrx/signals documentation:

  • Signal Store => SignalStore
  • Impersonal sentences

Not always a simple service 😅

Suggested change
A Signal Store is a simple Angular Service, so the same testing techniques you use for services apply to Signal Stores as well. This guide focuses on providing examples for common testing scenarios.
SignalStore is an Angular Service, so the same testing techniques used for services also apply to SignalStore.


A Signal Store is a simple Angular Service, so the same testing techniques you use for services apply to Signal Stores as well. This guide focuses on providing examples for common testing scenarios.

A challenging part of testing is knowing how to handle asynchronous tasks and mocking. The examples use Jest (v29.5), but the same principles apply to other testing frameworks.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main heading is missing in the beginning - # Testing. After the main title, we can put the second sentence from the previous section together with this:

# Testing

This page contains examples of common testing scenarios with NgRx SignalStore.
Examples use Jest, but the same principles apply to other testing frameworks.

// ... subheadings and the rest of the content

Note that the exact Jest version is removed. It's not necessary in my opinion as we're using Jest APIs available in most of the recent versions but also to avoid this guide looks obsolete after new Jest releases.


</div>

We use Jest's tools to manage asynchronous tasks. We avoid using `fakeAsync` or `waitForAsync` as they depend on zone.js and are incompatible with zoneless applications.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we know what's the status of Angular testing helpers - does the Angular team plan to deprecate them or provide Zoneless support when Zoneless mode becomes stable? If they don't plan to provide Zoneless support for the testing package, feel free to ignore my previous comment.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an in-between strategy, they allowed detectChanges to be called with a zoneless fixture. See this PR.

I believe a fake Zone is created and used behind the scenes for zoneless apps.

This issue could be of good interest to watch. It pertains to timers and zoneless.

Another PR that could be relevant is that fakeAsync will now automatically flush.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, @markostanimirovic, I just spotted your comment here and answered the above.

I think we should get a clarification from the official side first and then decide if we stick to fakeAsync or go with the native ones.

})
```

This version requires that the test can modify the state, even if it is protected.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
This version requires that the test can modify the state, even if it is protected.
This version requires the `MoviesStore` state to be unprotected.


This version requires that the test can modify the state, even if it is protected.

We are currently evaluating options for test helpers that allow state modification for protected states without introducing new mocking features that are already covered by existing testing frameworks.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence can be removed from the official guide. Opening an issue on this topic would be a better option in my opinion.


We are currently evaluating options for test helpers that allow state modification for protected states without introducing new mocking features that are already covered by existing testing frameworks.

Additionally, there is a community project by Gergely Szerovay that provides a comprehensive mocking library for Signal Stores. You can find more details here: https://www.angularaddicts.com/p/how-to-mock-ngrx-signal-stores
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here. To keep this guide concise, we can add the "Community Plugins" page and share community work there.

@@ -0,0 +1,687 @@
<!-- TOC -->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make this page visible in the side nav, we should add link to the navigation.json: https://github.com/ngrx/platform/blob/main/projects/ngrx.io/content/navigation.json#L311

Comment on lines 35 to 40
There are two main scenarios for testing:

1. Testing the `signalStore` itself,
2. Testing a component or service that uses the Signal Store.

In the first scenario, you’ll mock the dependencies of the Signal Store, while in the second scenario, you’ll mock the Signal Store itself.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a third scenario where we test the service by:

  1. creating a testing component
  2. injecting the service
  3. mocking only the service dependencies (not the signal store)

In this way, we can make assertions about what user behavior the service provides without testing the implementation details of the service.

Scenario one above tests pure implementation details and tells us very little about how the service will be used alongside user behavior. Scenario two above tests user behavior, but depending on how broad the scope of the service is may not result in 100% coverage of the branches/functions/etc of the service in that single component.

I think there's a good opportunity to present an opinion about how services can be tested in general (via this third scenario).

I think Tim is hinting at this in his comment about integration testing but I don't want to put words in his mouth

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I thought it is not necessary, but you two have convinced me. I will add it.

@rainerhahnekamp
Copy link
Contributor Author

@markostanimirovic, @timdeschryver, @jdegand, @markgoho

I've incorporated all your suggestions and left them open for your convenience. I'm now ready for the next review round and promise it will be faster as the first one. 😄

@brandonroberts
Copy link
Member

The formatting on some of the code snippets is a little off, but I think in general its ready to be merged, and we can follow up with additional PRs with more cleanup.

const fixture = TestBed.createComponent(MoviesComponent);
fixture.autoDetectChanges(true);

const studio = load.mock.calls[0][0];
Copy link

@jdegand jdegand Sep 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be wrong. Only one [0] ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[0][0] means the first parameter of the first call.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right. mock.calls returns an array of arrays.

@timdeschryver
Copy link
Member

@markostanimirovic, @timdeschryver, @jdegand, @markgoho

I've incorporated all your suggestions and left them open for your convenience. I'm now ready for the next review round and promise it will be faster as the first one. 😄

Thanks @rainerhahnekamp 🙏
I will go through this later this week (probably over the weekend)

Copy link
Member

@markostanimirovic markostanimirovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed formatting and issues with angle brackets.

There is room for improvement. For example, some code snippets are too long.

We can merge the testing guide as is for now and improve it later.

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left some grammar notes, for the rest it looks good and I like the structure you gave it.
(I agree with Marko's comment)

@rainerhahnekamp
Copy link
Contributor Author

Thanks @timdeschryver

Copy link
Member

@timdeschryver timdeschryver left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rainerhahnekamp !

@timdeschryver timdeschryver merged commit 1f7f740 into ngrx:main Sep 17, 2024
11 checks passed
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

Successfully merging this pull request may close these issues.

Add testing guide for NgRx SignalStore
6 participants