add firestore-backed application datastore integration test - #7077
ayushsarode wants to merge 7 commits into
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 canceled.
|
|
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.
| } | ||
| } | ||
|
|
||
| func TestUpdateApplication(t *testing.T) { |
| 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! |
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.