docs(docs-infra): remove deprecated module and add test cases #63497
Conversation
fe20113 to
c6b6e71
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the tests for ContentLoader to use modern Angular testing utilities and significantly expands test coverage. The new tests are comprehensive and cover success cases, caching logic, and various error scenarios. I've identified a couple of areas for improvement in the tests to make them more robust and maintainable. Overall, this is a great improvement to the test suite.
| it('should handle paths with different extensions correctly', async () => { | ||
| const testCases = [ | ||
| {input: 'document.txt', expected: 'document.txt'}, | ||
| {input: 'document.json', expected: 'document.json'}, | ||
| {input: 'document.xml', expected: 'document.xml'}, | ||
| {input: 'document', expected: 'document.md.html'}, | ||
| ]; | ||
|
|
||
| for (const testCase of testCases) { | ||
| const mockContent = `Content for ${testCase.input}`; | ||
|
|
||
| const contentPromise = service.getContent(testCase.input); | ||
|
|
||
| const req = httpTestingController.expectOne(`assets/content/${testCase.expected}`); | ||
| req.flush(mockContent); | ||
|
|
||
| const result = await contentPromise; | ||
| expect(result.id).toEqual(testCase.expected); | ||
| expect(result.contents).toEqual(mockContent); | ||
| } | ||
| }); |
There was a problem hiding this comment.
For data-driven tests, it's a better practice to generate separate it blocks for each test case, typically within a describe block. This improves test isolation and reporting. If one case fails, it won't stop the others, and the test runner will clearly indicate which specific case failed. I've also taken the liberty to combine the assertions for conciseness.
describe('should handle paths with different extensions correctly', () => {
const testCases = [
{input: 'document.txt', expected: 'document.txt'},
{input: 'document.json', expected: 'document.json'},
{input: 'document.xml', expected: 'document.xml'},
{input: 'document', expected: 'document.md.html'},
];
for (const {input, expected} of testCases) {
it(`should correctly handle '${input}' and request '${expected}'`, async () => {
const mockContent = `Content for ${input}`;
const contentPromise = service.getContent(input);
const req = httpTestingController.expectOne(`assets/content/${expected}`);
req.flush(mockContent);
const result = await contentPromise;
expect(result).toEqual({id: expected, contents: mockContent});
});
}
});There was a problem hiding this comment.
This is actually a good suggestion @MeAkib, can you convert the code in PR to this suggestion. Thank you.
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Does this PR introduce a breaking change?