-
Notifications
You must be signed in to change notification settings - Fork 59
feat: partial (API Keys, Audiences, and Contacts) non-mocked test coverage #663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
commit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
14 issues found across 40 files
Prompt for AI agents (all 14 issues)
Understand the root cause of the following 14 issues and fix them.
<file name="src/contacts/contacts.integration.spec.ts">
<violation number="1" location="src/contacts/contacts.integration.spec.ts:16">
Await polly.stop() so the afterEach hook doesn’t resolve before Polly finishes persisting recordings and shutting down adapters, which can leave flaky or incomplete recordings.</violation>
<violation number="2" location="src/contacts/contacts.integration.spec.ts:20">
Rule violated: **No `should` in tests**
Please rewrite this test description to avoid the word "should" and use a direct, declarative phrase instead (applies to this and the other new tests in this file).</violation>
</file>
<file name="src/audiences/__recordings__/Audiences-Integration-Tests-create-should-handle-validation-errors_2024921708/recording.har">
<violation number="1" location="src/audiences/__recordings__/Audiences-Integration-Tests-create-should-handle-validation-errors_2024921708/recording.har:3">
Rule violated: **No `should` in tests**
Rename this test description to avoid the word “should”; the project’s No `should` in tests rule requires declarative phrasing.</violation>
</file>
<file name="src/api-keys/__recordings__/API-Keys-Integration-Tests-create-should-create-an-API-key-with-sending-access_3080998429/recording.har">
<violation number="1" location="src/api-keys/__recordings__/API-Keys-Integration-Tests-create-should-create-an-API-key-with-sending-access_3080998429/recording.har:3">
Rule violated: **No `should` in tests**
The recorded test name still uses the word "should", which violates the "No `should` in tests" guideline. Rename the test description to a declarative phrase like "creates an API key with sending access" so the recording and test stay compliant.</violation>
</file>
<file name="src/audiences/audiences.integration.spec.ts">
<violation number="1" location="src/audiences/audiences.integration.spec.ts:16">
Await polly.stop() in the async afterEach so the Polly recorder finishes flushing recordings before the next test runs.</violation>
<violation number="2" location="src/audiences/audiences.integration.spec.ts:20">
Rule violated: **No `should` in tests**
Test descriptions must avoid the word "should" per the No `should` in tests guideline—please rephrase this case to a direct statement.</violation>
<violation number="3" location="src/audiences/audiences.integration.spec.ts:34">
Rule violated: **No `should` in tests**
Please rewrite this test description without "should" so it reads as a declarative statement, in line with the No `should` in tests rule.</violation>
<violation number="4" location="src/audiences/audiences.integration.spec.ts:44">
Rule violated: **No `should` in tests**
Rephrase this test title to remove "should" and state the behavior directly to comply with the No `should` in tests guideline.</violation>
<violation number="5" location="src/audiences/audiences.integration.spec.ts:70">
Rule violated: **No `should` in tests**
Adjust this test description to drop "should" and make the statement declarative, per the No `should` in tests requirement.</violation>
<violation number="6" location="src/audiences/audiences.integration.spec.ts:127">
Rule violated: **No `should` in tests**
Update this test description to a direct statement without "should", consistent with the No `should` in tests requirement.</violation>
</file>
<file name="src/api-keys/__recordings__/API-Keys-Integration-Tests-create-should-create-an-API-key-with-full-access_1681439770/recording.har">
<violation number="1" location="src/api-keys/__recordings__/API-Keys-Integration-Tests-create-should-create-an-API-key-with-full-access_1681439770/recording.har:47">
The recorded response stores the full Resend API token in plain text, leaking a sensitive secret. Please redact the token value before committing the cassette.</violation>
</file>
<file name="package.json">
<violation number="1" location="package.json:35">
Use double quotes around the glob so rimraf works on Windows; otherwise the pattern is passed with literal single quotes and the cleanup step fails on that platform.</violation>
</file>
<file name="src/api-keys/api-keys.integration.spec.ts">
<violation number="1" location="src/api-keys/api-keys.integration.spec.ts:16">
Await polly.stop() inside the async afterEach so its Promise settles before the next test begins; otherwise teardown can overlap and recordings may not persist correctly.</violation>
</file>
<file name="src/test-utils/polly-setup.ts">
<violation number="1" location="src/test-utils/polly-setup.ts:4">
Rule violated: **Initialisms and Acronyms Naming Conventions**
Rename the imported identifier so the `FS` acronym is camel-cased (e.g., `FsPersister`) to comply with the Initialisms and Acronyms Naming Conventions rule; update its usages accordingly.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
.../Audiences-Integration-Tests-create-should-handle-validation-errors_2024921708/recording.har
Outdated
Show resolved
Hide resolved
...tegration-Tests-create-should-create-an-API-key-with-sending-access_3080998429/recording.har
Outdated
Show resolved
Hide resolved
|
Thanks for the review, Cubic! I'll address those this evening. |
04e9557 to
7bd7fbf
Compare
3ac88cd to
54d2cd5
Compare
|
This looks amazing, that's exactly a problem I was thinking of for a bit, thank you! |
54d2cd5 to
56202eb
Compare
| }); | ||
|
|
||
| // Redact API keys from recordings before saving them | ||
| polly.server.any().on('beforePersist', (_, recording) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
love this!
|
Glad it's filling a need! 😊 |
|
Hey @gabrielmfern, low priority, but would y'all be up for adding the "hacktoberfest" topic to this repo? Trying to get my numbers up over there. 😅 (Though that's far from the main reason I'm contributing.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try it out!
This PR adds integration tests that record and replay real Resend API responses using Polly.JS. These tests complement the existing unit tests by verifying actual HTTP interactions.
Why?
The current unit tests mock all API responses, which doesn't catch certain bugs:
DELETErequests #635) where DELETE requests didn't send the Authorization header, causing401 Unauthorizederrors. Unit tests didn't catch this because they didn't verify actual HTTP requests.How It Works
Test Modes
Default (Replay Mode)
pnpm testTests use pre-recorded API responses from
__recordings__directories (located alongside each integration test file). Fast and deterministic, no API key needed. Ideal for CI/CD workflows, so they don't have to make their own network requests.Re-record All Tests
Makes real API calls and saves responses to
__recordings__directories. Useful after API changes. Requires a validRESEND_API_KEY.Dev Mode (Record If Missing)
Uses existing recordings when available, records new ones when missing. Useful when adding new tests.
Setting API Key
You can set
RESEND_API_KEYeither in your shell or in a.env.testfile (loaded via dotenv invitest.setup.mts):Note: The dotenv setup is optional and can be removed from this PR if preferred. I just found it convenient when developing these tests.
Security: API Key Redaction
Polly.JS is configured to automatically redact API keys before saving recordings (see
polly-setup.ts): This means:Test Coverage
Integration tests currently cover API Keys, Audiences, and Contacts. They do catch the DELETE headers bug if I back out the fix in #635.
Additional SDK areas (domains, emails, broadcasts) would be best added by the Resend team, since test domains would need to be part of the test code. Once those recordings are committed, though, anyone can replay them without account access to those domains.