feat(Inso): add an overall run collection test result summary - #8739
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds a new test-result reporter module and integrates it into the CLI, extracting reporter logic, summarizing results across requests, and covering behaviors with unit tests.
- Extract reporter logic from
cli.tsintoreporter/index.ts - Introduce
logTestResultSummaryto aggregate results over multiple requests - Update CLI to use the new reporter functions and remove old inline reporter code
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/insomnia-inso/src/reporter/index.ts | New module defining reporters and summary logic |
| packages/insomnia-inso/src/reporter/index.test.ts | Tests covering each reporter type and summary |
| packages/insomnia-inso/src/reporter/snapshots | Snapshots validating reporter output formatting |
| packages/insomnia-inso/src/cli.ts | Removed inline reporter code and now calling new reporter functions |
| packages/insomnia-inso/package.json | Added picocolors dependency for colored output |
Comments suppressed due to low confidence (1)
packages/insomnia-inso/src/reporter/index.ts:50
- [nitpick] The name
fallbackReporteris somewhat generic. Renaming it to something likelistOutputordefaultReporterOutputcould make its purpose clearer.
const fallbackReporter = testResults.map(r => `${r.status === 'passed' ? '✅' : '❌'} ${r.testCase}`).join('\n');
| "consola": "^2.15.3", | ||
| "cosmiconfig": "^9.0.0", | ||
| "enquirer": "^2.4.1", | ||
| "picocolors": "^1.1.1", |
There was a problem hiding this comment.
have you evaluated this dep for maintainability?
There was a problem hiding this comment.
@jackkav Yes, it has 1.5k stars and fewer than ten issues. Its npm weekly downloads is 84 million. And it's a dependent of mocha, postcss, ...etc.
Compare with chalk, it's much smaller and quicker.
| ); | ||
| let success = true; | ||
|
|
||
| const testResultsQueue: RequestTestResult[][] = []; |
There was a problem hiding this comment.
I think queue may be a misleading name here, its an array of test results, test result list is as ambiguous without implying that it has any special queue processing step
There was a problem hiding this comment.
Yes, I was thinking about the future reporter design and then named it as queue, didn't notice it. Has been renamed to testResultsList.
| beforeEach(() => { | ||
| console.log = mockConsoleLog; | ||
| mockConsoleLog.mockClear(); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| console.log = originalConsoleLog; | ||
| }); |
There was a problem hiding this comment.
I generally consider mocking a code smell, heres how I would address this particular one.
Is it reasonable to modify the function such that it can be responsible for less? In this case generate the output and return a string to test, rather than generate the output and also log it.
This would also improve the quality of the code as it would be a pure function which is more composable and extensible.
Another implementation option would be to pass a log function down rather than returning a string.
Both ways you can avoid managing mock state in your test and eliminate the cruft in your test assertions, which cuts down the amount of code in your tests, and improves readability.
There was a problem hiding this comment.
Good point. I’ve added a log parameter to accept a logging function and removed the mock.
I'm also considering mocking it globally — otherwise, in some cases, we might have to modify the production code just to make the test work, which is something we should avoid.
jackkav
left a comment
There was a problem hiding this comment.
Nice work extending the feature, couple of comments to address but great start.
|
Hi @jackkav, thanks for the comments. I made some changes based on them. Could you take another look? Thank you. 🙏 |
Background
INS-5567, part of Kong/insomnia-docs#273
Changes
Before
After