Conversation
There was a problem hiding this comment.
The PR refactors test configuration from inline process.env overrides to CDS profiles (.cdsrc.json), which is a clean improvement. The main issues found are: two copy-paste mistakes in REVISIT comments (wrong profile name referenced), and the in-memory-outbox test dropping the critical outbox: { kind: 'in-memory-outbox' } setting from its env override, which may cause that test case to not exercise the intended code path.
PR Bot Information
Version: 1.26.11
- LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
ffefc4d8-0bc2-4c85-beed-2debefc0d843 - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
There was a problem hiding this comment.
The PR is a well-structured refactoring that centralises test configuration into .cdsrc.json profiles and drops CDS v8 matrix support. Two issues were raised: the missing in-memory-outbox profile (no test coverage for that scenario) and the potential race condition in metrics-outbox.test.js where the snapshot may already reflect retry-2 side effects due to ordering of the wait and snapshot lines.
PR Bot Information
Version: 1.26.11
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review - File Content Strategy: Full file content
- Correlation ID:
e2544745-64ee-46f6-aa91-9cadcee6c0b4
| while (currentRetryCount[E2] < 2) await wait(100) | ||
| await wait(300) // ... for the retry to be processed and metrics to be collected |
There was a problem hiding this comment.
Logic Error: The snapshot at line 182-183 is taken after await wait(300) at line 176, meaning E2's retry has already been processed and totalOut/totalFailed may already reflect the extra increment from retry 2. The snapshot should be taken before the 300ms wait to reliably capture the state before the retry fires.
| while (currentRetryCount[E2] < 2) await wait(100) | |
| await wait(300) // ... for the retry to be processed and metrics to be collected | |
| while (currentRetryCount[E2] < 2) await wait(100) | |
| // Snapshot counters now: retry 2 fires at t+1250ms (waitingTime(2)), which overlaps | |
| // with the wait below, causing totalOut/totalFailed to advance ahead of the metric export | |
| const snapOut = { ...totalOut } | |
| const snapFailed = { ...totalFailed } | |
| await wait(300) // ... for the retry to be processed and metrics to be collected |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
There was a problem hiding this comment.
Keep this for later, once cds-version will be [9, 10]?
Upgrade Test Suite for CDS v10 Compatibility
Refactor
♻️ Refactored the test suite to align with CDS v10 conventions, migrating from inline
process.envconfiguration overrides to CDS profiles and a dedicated.cdsrc.jsonfile. The CI pipeline matrix is also narrowed to CDS v9 only, with v10 commented out as a future target.Changes
.github/workflows/ci.yml: Removed CDS v8 from the test matrix (now v9 only); removed v8-specific install steps; added a commented-out v10 entry for future use..gitignore: Removed the ignore rule fortest/bookshop/.cdsrc.json(now tracked); added.claude/to the ignore list.test/bookshop/.cdsrc.json: New file — Centralizes CDS profile-based configuration forlogging,metrics,metrics-outbox,metrics-outbox-disabled,tracing-attributes,persistent-outbox, andwithout-outboxprofiles, replacing scatteredprocess.envoverrides.test/bookshop/package.json: Added@sap/cds-dkas a dev dependency; removed_outboxentry and[metrics-outbox]/[metrics-outbox-disabled]profile blocks (moved to.cdsrc.json); added_schedulingflag.test/logging.test.js: Removed inlineprocess.envtelemetry/logging config overrides; switched to--profile logging. Retained a minimalcds_logenv override with a note aboutcls_custom_fieldsprofile precedence limitation.test/metrics.test.js: Removedprocess.env.cds_requires_telemetry_metrics_configoverride; switched to--profile metrics.test/metrics-outbox.test.js: Fixed a race condition by waiting onE2instead ofE1for retry tracking; extended wait time; snapshots counter values before assertions to avoid flaky comparisons.test/tracing-attributes.test.js: Removedprocess.envexporter override; switched to--profile tracing-attributes.test/tracing-messaging-persistent-outbox.test.js: RenamedCASEtopersistent-outbox; removed redundantprocess.envoverrides; skipped the describe block pending vitest migration.test/tracing-messaging-without-outbox.test.js: RenamedCASEtowithout-outbox; fixed messaging config to useoutboxed: falseinstead ofoutbox: false.test/tracing-messaging-with-in-memory-outbox.test.js: Removed — in-memory queues are not supported in CDS v10.test/tracing-messaging.js: Switched fromcds.test().in(...)tocds.test(..., '--profile', CASE).test/tracing.test.js: Switched fromcds.test().in(...)tocds.test(...).PR Bot Information
Version:
1.26.1103cf6a2a-ae17-41bf-af02-c44adf8f5639pull_request.editedanthropic--claude-4.6-sonnet