Fork Fix - Adjust unit tests - #501
Conversation
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR replaces a carefully controlled jest.mock for @sap/cds with real-module spies, but the substitution has several correctness gaps: the console.error spy does not intercept LOG.error calls from the CDS logger, making the "no leakage" assertions vacuously passing; the "..." containment check is too broad to prove redaction; and the spy on cds.utils.redacted may throw or misbehave if the real module doesn't expose that function in the test environment. The overall effect is a test suite that appears to cover credential-leakage protection but no longer reliably detects regressions.
PR Bot Information
Version: 1.29.2
- LLM:
anthropic--claude-4.6-sonnet - File Content Strategy: Full file content
- Event Trigger:
pull_request.opened - Experiment Variant:
control - Correlation ID:
3aa774b0-8a6c-11f1-91bc-a8296c97508a
| redactedSpy = jest.spyOn(cds.utils, "redacted") | ||
| consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {}) |
There was a problem hiding this comment.
Logic Error: consoleErrorSpy intercepts console.error, but validateServiceManagerCredentials logs via LOG.error (a cds.log instance), not console.error directly.
The production code calls LOG.error(...) where LOG = cds.log("attachments"). Under the real @sap/cds (no longer mocked with jest.mock), cds.log may or may not forward to console.error, and the spy is set up after LOG is already bound at module load time. This means consoleErrorSpy.mock.calls will almost certainly be empty regardless of what validateServiceManagerCredentials does, making the "Verify raw secrets never reached console" assertions vacuously true — they can never fail even if secrets are leaked.
The original approach of spying on mockLogInstance.error directly was precise and reliable. If the goal is to avoid a full jest.mock, consider at minimum spying on the cds.log return value, or use jest.spyOn(cds, 'log') to return a controlled logger instance whose .error calls can be inspected.
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
| expect(loggedCreds.sm_url).toBe("https://sm.example.com") | ||
| expect(loggedCreds.url).toBe("https://token.example.com") | ||
| // Redacted values should appear instead | ||
| expect(loggedString).toContain("...") |
There was a problem hiding this comment.
Logic Error: The "..." containment check is an overly broad assertion that can produce a false positive.
JSON.stringify(consoleErrorSpy.mock.calls) will contain "..." if anything in the serialized call args contains that substring — including the error message "Missing Service Manager credentials", the key "sm_url", or any other incidental string. This does not actually verify that cds.utils.redacted masked the secret fields. The original assertion expect(loggedCreds.clientsecret).toBe("...") was precise and directly confirmed the redaction contract.
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
| beforeEach(() => { | ||
| mockLogInstance.error.mockClear() | ||
| mockRedacted.mockClear() | ||
| redactedSpy = jest.spyOn(cds.utils, "redacted") |
There was a problem hiding this comment.
Bug: redactedSpy is set up with jest.spyOn(cds.utils, "redacted") but without .mockImplementation(...), so the real cds.utils.redacted executes. However, in the new setup there is no jest.mock("@sap/cds", ...) at the top of the file, meaning cds is the actual @sap/cds module. If cds.utils.redacted is undefined or behaves differently in the test environment, the spy setup will throw, and the assertion expect(redactedSpy).toHaveBeenCalledWith(sensitiveCredentials) may fail for the wrong reason. Ensure cds.utils.redacted exists in the test environment, or provide a fallback mock implementation.
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
|
This is a duplicate of Pull #500 |
2 participants
Refactor Unit Tests: Replace Manual Mocks with Jest Spies for
@sap/cdsRefactor
♻️ Refactored unit tests to remove manual mock definitions for
@sap/cdsand replace them with proper Jest spy-based approaches, improving test reliability and alignment with how the module is actually used.Changes
tests/unit/unitTests.test.js:mockLogInstanceandmockRedactedmock objects, along with thejest.mock("@sap/cds", ...)block that overrode the entire module.@sap/cdsis nowrequired directly afterjest.doMocksetup, allowing tests to interact with the real module shape.validateServiceManagerCredentials - no credential leakagedescribe block, replacedmockLogInstance.errorandmockRedactedusage withjest.spyOnoncds.utils.redactedandconsole.error.beforeEachandafterEachhooks to set up and restore spies cleanly between tests.cds.utils.redactedwas called with the sensitive credentials and that raw secret values never appear inconsole.erroroutput, rather than asserting on specific mock call arguments or object property values.PR Bot Information
Version:
1.29.2anthropic--claude-4.6-sonnetpull_request.opened3aa774b0-8a6c-11f1-91bc-a8296c97508a