Test: Run Integration project in code coverage check#883
Open
Test: Run Integration project in code coverage check#883
Conversation
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The merge-ctrf.js script assumes both UI and integration CTRF files are present and valid; consider adding existence checks and a clearer failure message or fallback behavior if one of the reports is missing or empty to make CI failures easier to understand.
- In mergeSummary, using
Infinityas the default forstartcan result instart: Infinityif both inputs lack a start time; it would be more robust to omitstartor default toundefinedwhen no valid timestamps are available.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The merge-ctrf.js script assumes both UI and integration CTRF files are present and valid; consider adding existence checks and a clearer failure message or fallback behavior if one of the reports is missing or empty to make CI failures easier to understand.
- In mergeSummary, using `Infinity` as the default for `start` can result in `start: Infinity` if both inputs lack a start time; it would be more robust to omit `start` or default to `undefined` when no valid timestamps are available.
## Individual Comments
### Comment 1
<location path=".github/scripts/merge-ctrf.js" line_range="19-31" />
<code_context>
+ return JSON.parse(raw);
+}
+
+function mergeSummary(a, b) {
+ const sum = (x, y) => (x ?? 0) + (y ?? 0);
+ return {
+ tests: sum(a.tests, b.tests),
+ passed: sum(a.passed, b.passed),
+ failed: sum(a.failed, b.failed),
+ pending: sum(a.pending, b.pending),
+ skipped: sum(a.skipped, b.skipped),
+ other: sum(a.other, b.other),
+ start: Math.min(a.start ?? Infinity, b.start ?? Infinity),
+ stop: Math.max(a.stop ?? 0, b.stop ?? 0),
+ };
+}
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid emitting `Infinity`/`0` timestamps in merged summary when both inputs lack `start`/`stop`.
With both `a.start` and `b.start` undefined, `Math.min(a.start ?? Infinity, b.start ?? Infinity)` becomes `Infinity`, and `stop` can similarly become `0` when both are missing, which corrupts the merged summary. Consider explicitly handling the case where both are undefined, e.g. collect defined values with `const starts = [a.start, b.start].filter(v => v != null);` and only call `Math.min(...starts)` when non‑empty; otherwise leave `start` (and `stop`) undefined.
```suggestion
function mergeSummary(a, b) {
const sum = (x, y) => (x ?? 0) + (y ?? 0);
const starts = [a.start, b.start].filter((v) => v != null);
const stops = [a.stop, b.stop].filter((v) => v != null);
return {
tests: sum(a.tests, b.tests),
passed: sum(a.passed, b.passed),
failed: sum(a.failed, b.failed),
pending: sum(a.pending, b.pending),
skipped: sum(a.skipped, b.skipped),
other: sum(a.other, b.other),
start: starts.length ? Math.min(...starts) : undefined,
stop: stops.length ? Math.max(...stops) : undefined,
};
}
```
</issue_to_address>
### Comment 2
<location path=".github/workflows/playwright-actions.yml" line_range="225-234" />
<code_context>
+ - name: Append integration test env vars to .env
</code_context>
<issue_to_address>
**🚨 issue (security):** Be mindful of persisting sensitive credentials into a `.env` file in CI.
This writes several usernames, passwords, and activation keys into `.env` in the workspace. If that file is ever committed, cached, or uploaded as an artifact/log, secrets could leak. If these values are only needed for the integration run, consider passing them via the Playwright step `env:` or `$GITHUB_ENV` instead of a persistent `.env` file, and/or ensure `.env` is explicitly cleaned up after tests finish.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
6fd48b2 to
e7107f3
Compare
e7107f3 to
ad05211
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add Integration project to code coverage check
Testing steps
tests pass, results are updated in currents.dev
Depends on: Test: code coverage log to currents #861