docs: modernize testing services guide#67180
Conversation
ed01617 to
68ef561
Compare
68ef561 to
4941678
Compare
|
|
||
| To test this service, configure a `TestBed`, which is Angular's testing utility for creating an isolated testing environment for each test. It sets up dependency injection and lets you retrieve service instances — simulating how Angular wires things together in a real application. | ||
|
|
||
| ```ts { header: 'calculator.unit.ts' } |
There was a problem hiding this comment.
a little nit , according style guide
| ```ts { header: 'calculator.unit.ts' } | |
| ```ts { header: 'calculator.spec.ts' } |
yjaaidi
left a comment
There was a problem hiding this comment.
Thank for this, @bencodezen! This is really moving the testing docs in the right direction.
My feedback is mostly about how the docs might nudge users into writing over-specifying tests.
Also, maybe the example could use a fake instead of stub. It has the side effect of keeping this focused on Angular APIs and less about Vitest features.
Spies and stubs could appear later in a different chapter.
What do you think?
| expect(value).toBe('observable value'); | ||
| beforeEach(() => { | ||
| // Sets up a TestBed with no dependencies at this time | ||
| TestBed.configureTestingModule({}); |
There was a problem hiding this comment.
TestBed#configureTestingModule could be introduced later as it's not required in this case.
Or maybe, the docs could just mention that this calls is not necessary if there is nothing to configure.
|
|
||
| As a service _consumer_, you don't worry about any of this. | ||
| You don't worry about the order of constructor arguments or how they're created. | ||
| Most services depend on other services to run properly. When testing these services, controlling these dependencies enables you to ensure that the tests stay focused on the service's own logic. |
There was a problem hiding this comment.
I really don't mean to be pedantic but I think that doc example can nudge the testing strategy more than one could think of.
I am afraid that this sentence controlling these dependencies enables you to ensure that the tests stay focused nudges users to write "over-narrow" tests.
I would make it feel like an opt-in like sometimes, you might want to exclude some of these dependencies...
More about this:
In other words, replacing dependencies with test doubles does not necessarily change the focus.
The System Under Test could be the OrderTotal while the exercised code is OrderTotal + TaxCalculator. For example, Martin Fowler calls these sociable unit-tests.
Actually, replacing dependencies often harms the focus because the developer's cognitive load is over-loaded with the maintenance of the test double. Unless, when the test double is a reusable fake 😇
| expect(service.getValue()).toBe('real value'); | ||
| }); | ||
| ``` | ||
| In this example, `OrderTotal` uses `inject()` to request `TaxCalculator` from Angular's dependency injection system. In production, Angular provides the real implementation. However, since your focus is testing the `OrderTotal` service, you can substitute it with a controlled replacement to isolate `OrderTotal`'s logic. |
There was a problem hiding this comment.
Related to my previous comment maybe this could just mention that the complexity of TaxCalculator makes us want to substitute it.
The idea is to nudge users to really think twice before using a test double, not just replace any dependencies to focus on the "thing" they are testing.
| In this example, `OrderTotal` uses `inject()` to request `TaxCalculator` from Angular's dependency injection system. In production, Angular provides the real implementation. However, since your focus is testing the `OrderTotal` service, you can substitute it with a controlled replacement to isolate `OrderTotal`'s logic. | ||
|
|
||
| Or inside the `beforeEach()` if you prefer to inject the service as part of your setup. | ||
| ### Replacing a dependency with a stub |
There was a problem hiding this comment.
🙏🙏🙏 thank you for using the word stub and never abusing the word mock in this doc.
| ```ts | ||
| let masterService: MainService; | ||
| let valueServiceSpy: Mocked<ValueService>; | ||
| const taxCalculatorStub = { |
There was a problem hiding this comment.
What about typing this? Pedagogically speaking it's arguable to do it here or keep it for later but not typing the test doubles is a very common source of trouble.
| // Verify the interaction with a spy | ||
| it('passes the subtotal to the tax calculator', () => { | ||
| service.total(100); | ||
| expect(taxCalculatorStub.calculate).toHaveBeenCalledWith(100); |
There was a problem hiding this comment.
Maybe toHaveBeenCalledExactlyOnce instead.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
https://angular.dev/guide/testing/services
Issue Number: N/A
What is the new behavior?
Modernizes the "Testing with services" guide in preparation for an upcoming guide that will cover "Testing with dependency injection" on a broader level.
Does this PR introduce a breaking change?
Other information