test(api): add API tests for importers - #1219
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1219 +/- ##
=======================================
Coverage 54.19% 54.19%
=======================================
Files 255 255
Lines 5715 5715
Branches 1774 1774
=======================================
Hits 3097 3097
Misses 2355 2355
Partials 263 263
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Reviewer's GuideAdds isolated Playwright/Axios API tests covering importer creation, retrieval, PATCH and PUT updates with revision headers, and deletion, including response-status and persistence assertions. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="e2e/tests/api/features/importer.ts" line_range="68" />
<code_context>
+ },
+ },
+ );
+ expect(patchResponse.status).toBe(204);
+
+ const verifyResponse = await axios.get(
</code_context>
<issue_to_address>
**issue (bug_risk):** These tests require PATCH, PUT, and DELETE to return 204, but the checked-in OpenAPI contract declares 201 for all three importer operations, so the tests fail against an implementation that follows the documented API even when the mutation succeeds.
**Suggested fix:** Align the assertions with the actual API contract, or update the API/OpenAPI contract if 204 is the intended response.
</issue_to_address>
### Comment 2
<location path="e2e/tests/api/features/importer.ts" line_range="137" />
<code_context>
+ validateStatus: () => true,
+ })
+ .catch(() => undefined);
+ await axios.post(`/api/v3/importer/${importerName}`, BASE_IMPORTER_CONFIG);
+
+ try {
</code_context>
<issue_to_address>
**issue (bug_risk):** The delete test has no `try`/`finally` cleanup, so any failure after creation leaves `api-test-delete-importer` persisted in the environment; the next run's pre-delete hides the leftover state, but a failed run pollutes shared test data and can affect other consumers.
**Triggers:** When the creation or any assertion in the delete test fails before the explicit delete completes.
**Suggested fix:** Put the create/read/delete assertions in a `try` block and delete the importer in `finally`, as the create, PATCH, and PUT tests do.
</issue_to_address>
### Comment 3
<location path="e2e/tests/api/features/importer.ts" line_range="17-21" />
<code_context>
+ test("Create importer and verify it exists", async ({ axios }) => {
+ const importerName = "api-test-create-importer";
+
+ await axios
+ .delete(`/api/v3/importer/${importerName}`, {
+ validateStatus: () => true,
+ })
+ .catch(() => undefined);
+
+ try {
</code_context>
<issue_to_address>
**issue (bug_risk):** The initial cleanup deletes a fixed importer name and suppresses every failure, then creation proceeds; if deletion is rejected by authorization, unavailable due to a transient server error, or otherwise does not remove an existing importer, the subsequent POST fails with a conflict rather than establishing the test's clean fixture.
**Triggers:** When a stale importer with the fixed name exists and the cleanup DELETE does not actually succeed.
**Suggested fix:** Use unique names per run, or verify the cleanup response and fail explicitly when an existing fixture cannot be removed.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and the change only adds API tests, so it does not alter production behavior directly. During execution the tests persist importer records and delete them afterward; if a test or cleanup fails, a bounded leftover importer may remain, but it can be removed or the test rerun and reverting the PR prevents further creation.
Blocking findings: e2e/tests/api/features/importer.ts:68, e2e/tests/api/features/importer.ts:137, e2e/tests/api/features/importer.ts:21
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| }, | ||
| }, | ||
| ); | ||
| expect(patchResponse.status).toBe(204); |
There was a problem hiding this comment.
issue (bug_risk): These tests require PATCH, PUT, and DELETE to return 204, but the checked-in OpenAPI contract declares 201 for all three importer operations, so the tests fail against an implementation that follows the documented API even when the mutation succeeds.
Suggested fix: Align the assertions with the actual API contract, or update the API/OpenAPI contract if 204 is the intended response.
| validateStatus: () => true, | ||
| }) | ||
| .catch(() => undefined); | ||
| await axios.post(`/api/v3/importer/${importerName}`, BASE_IMPORTER_CONFIG); |
There was a problem hiding this comment.
issue (bug_risk): The delete test has no try/finally cleanup, so any failure after creation leaves api-test-delete-importer persisted in the environment; the next run's pre-delete hides the leftover state, but a failed run pollutes shared test data and can affect other consumers.
Triggers: When the creation or any assertion in the delete test fails before the explicit delete completes.
Suggested fix: Put the create/read/delete assertions in a try block and delete the importer in finally, as the create, PATCH, and PUT tests do.
| await axios | ||
| .delete(`/api/v3/importer/${importerName}`, { | ||
| validateStatus: () => true, | ||
| }) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
issue (bug_risk): The initial cleanup deletes a fixed importer name and suppresses every failure, then creation proceeds; if deletion is rejected by authorization, unavailable due to a transient server error, or otherwise does not remove an existing importer, the subsequent POST fails with a conflict rather than establishing the test's clean fixture.
Triggers: When a stale importer with the fixed name exists and the cleanup DELETE does not actually succeed.
Suggested fix: Use unique names per run, or verify the cleanup response and fail explicitly when an existing fixture cannot be removed.
2 participants
Simple CRUD tests for the Importer feature, as these paths are not covered (and currently cannot be covered) by our UI suite.
Summary by Sourcery
Tests: