Canon: enforce reporter idempotency and record PASS evidence - #17
Conversation
Summary of ChangesHello @andreidita22, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly strengthens the idempotency of reporterWrite by ensuring duplicate messages are not appended, even with interleaved writes. The changes to the implementation and the corresponding test updates are logical and improve the robustness of the reporter. I've identified a potential performance concern in reporterWrite due to reading the entire log file on each call, and a minor code inconsistency in the test runner. Overall, these are solid improvements.
| const seen = new Set(prior); | ||
| const alreadyLogged = seen.has(trimmed); |
There was a problem hiding this comment.
This new logic correctly ensures idempotency by checking the entire log history. However, it's worth noting that this approach, which relies on reading the full log file and creating a Set on every reporterWrite call, could introduce a performance bottleneck if the log files become very large. While this is acceptable for now, it's a trade-off to keep in mind for future scaling.
| assert.equal(lines[0], spec.message); | ||
| const lines = contents | ||
| .split(/\r?\n/) | ||
| .map((line) => line.trim()) |
There was a problem hiding this comment.
To maintain consistency with the normalizeLines function in modules/reporter/src/index.js, it's better to use .trimEnd() here instead of .trim(). While it may not affect the outcome of this specific test because input messages are trimmed, aligning the logic prevents potential future discrepancies and improves code consistency.
| .map((line) => line.trim()) | |
| .map((line) => line.trimEnd()) |
Summary
reporterWriteskips appending log entries that already exist in the artifactTesting
https://chatgpt.com/codex/tasks/task_e_68f44dfe59248320a19f272e7edb3ab8