add firestore-backed application datastore integration test - #7077
Conversation
Signed-off-by: ayushsarode <ayushsarode777@gmail.com>
|
👋 Hi @ayushsarode, welcome to PipeCD and thanks for opening your first pull request! We’re really happy to have you here Before your PR gets merged, please check a few important things below. Helpful resources
DCO Sign-offAll commits must include a In case you forget to sign-off your commit(s), follow these steps: For the last commit: git commit --amend --signoff
git push --force-with-leaseFor multiple commits: git rebase --signoff origin/master
git push --force-with-leaseRun checks locallyBefore pushing updates, please run: make checkThis runs the same checks as CI and helps catch issues early. 💬 Need help?If anything is unclear, feel free to ask in this PR or join us on the CNCF Slack in the #pipecd channel. Thanks for contributing to PipeCD! ❤️ |
|
@rahulshendre please lmk if this integration test PR adds value |
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 7 days with no activity. Feel free to reopen if still applicable. |
✅ Deploy Preview for pipecd-site ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hi @ayushsarode, sorry for late reply Please consider adding test for Also I found out that the added tests are near identical versions of generic tests from firestore_test.go |
There was a problem hiding this comment.
🟡 Changes recommended
Add listing coverage and verify persisted fields after successful creation and update.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Firestore-backed integration tests for application datastore behavior.
Changes:
- Tests retrieval, creation, duplicate handling, and updates.
- Covers error and success paths against Firestore.
- Listing coverage and persistence assertions remain unresolved.
File summaries
| File | Summary |
|---|---|
test/integration/datastore/firestore/application_test.go |
Adds Firestore application datastore integration tests. |
Review details
Suppressed comments (1)
test/integration/datastore/firestore/application_test.go:124
- The successful create case only checks that
Createreturned nil; it never reads the document back. A Firestore write or serialization bug could therefore pass this test, and the successful case also usesid-newwhile the entity still hasId: "create-id". Fetch the new document and assert its stored fields (or make the entity ID match the document ID).
for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
err := store.Create(ctx, col, tc.id, fakeApplication)
assert.Equal(t, tc.wantErr, err)
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for _, tc := range testcases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| err := store.Update(ctx, col, tc.id, tc.updater) | ||
| assert.Equal(t, tc.wantErr, err) | ||
| }) | ||
| } |
Replace redundant Application CRUD tests with Find coverage matching the MySQL integration test, since generic CRUD is already covered in firestore_test.go. Signed-off-by: ayushsarode <ayushsarode777@gmail.com>
|
@armistcxy could you please review it now? |
|
wait, I've asked you to add the |
|
Hi @armistcxy , my apologies for that! |
|
@ayushsarode you forgot to sign off your commit |
Signed-off-by: ayushsarode <ayushsarode777@gmail.com>
4b27d5c to
421eb58
Compare
|
@armistcxy could you please check now? |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The added tests don’t cover delete/disable semantics (or other ApplicationStore contract behaviors) despite the PR description/issue stating they are included, so the scope needs to be expanded or the description corrected.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (1)
| func TestCreateApplication(t *testing.T) { | ||
| col := &collection{kind: "Application"} | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
There was a problem hiding this comment.
@ayushsarode can you add tests for Delete, Enable, Disable
Signed-off-by: ayushsarode <ayushsarode777@gmail.com>
|
@armistcxy could you check it now? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7077 +/- ##
=======================================
Coverage 30.36% 30.36%
=======================================
Files 601 601
Lines 52622 52622
=======================================
Hits 15980 15980
Misses 35123 35123
Partials 1519 1519
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:
|
rahulshendre
left a comment
rahulshendre
left a comment
There was a problem hiding this comment.
@ayushsarode, please sign off your commit.
@mohammedfirdouss, please drop your review as well if you get time 😄
3ce4efc to
db6c822
Compare
|
@rahulshendre signed off! |
|
Thank you for contributing to PipeCD, @ayushsarode! The changes in this pull request will be part of the upcoming release! |
What this PR does:
Adds integration test coverage for the Firestore-backed application datastore.
The tests cover core application datastore behavior against Firestore, including creating, retrieving, listing, updating, and deleting application records where applicable. This follows the existing datastore integration test pattern used in the repository.
Why we need it:
Application data is a core part of PipeCD’s datastore layer, and Firestore-specific behavior can differ from unit-test assumptions around document structure, queries, updates, and serialization.
Adding integration coverage helps catch regressions earlier and gives maintainers more confidence that the Firestore implementation stays consistent with the expected datastore contract.
Which issue(s) this PR fixes:
Fixes #7076
Does this PR introduce a user-facing change?:
No.
How are users affected by this change:
Users are not directly affected. This is a test-only change that improves confidence in Firestore datastore behavior.
Is this breaking change:
No.
How to migrate (if breaking change):
Not applicable.