[CTM-385] Create default firestore database if does not exist and Update Integration Test#2076
Open
[CTM-385] Create default firestore database if does not exist and Update Integration Test#2076
Conversation
snf2ye
force-pushed
the
sh/test-if-connected-tet-time-decreases
branch
2 times, most recently
from
12563c9 to
2f27a25
Compare
snf2ye
changed the title
snf2ye
force-pushed
the
sh/test-if-connected-tet-time-decreases
branch
from
b7f8ce9 to
db32e06
Compare
snf2ye
changed the title
snf2ye
commented
Mar 6, 2026
| .setProjectId(googleProjectId) | ||
| .build() | ||
| .getService(); | ||
| return FirestoreOptions.newBuilder().setProjectId(googleProjectId).build().getService(); |
Contributor
Author
There was a problem hiding this comment.
This is a change to use the same logic as we have in FirestoreProject
snf2ye
force-pushed
the
sh/test-if-connected-tet-time-decreases
branch
from
0d94d26 to
680a221
Compare
snf2ye
commented
Mar 9, 2026
| // Fetch BQ Dataset | ||
| // Fetch BQ Dataset with retry logic | ||
| String bqDatasetName = BigQueryPdao.prefixName(datasetName); | ||
| Dataset bqDataset = bigQuery.getDataset(bqDatasetName); |
Contributor
Author
There was a problem hiding this comment.
The SnapshotPermissionsIntegrationTest.snapshotAclTest() is consistently failing on this check. It can take a minute for the ACL (access control list) on the big query dataset to update. So, if we wrap this in a retry, the test can then pass. We use a retry in the production code, it was just not added to the test code.
Ideally, we would just call the bigQueryProject.getBQDataset(bqDatasetName) method, but this test is trying to specifically test that a big query instance with a given token has access to BQ.
snf2ye
changed the title
snf2ye
requested review from
calypsomatic and
rjohanek
and removed request for
a team
Contributor
Author
|
@claude please review for critical issues |
Update GoogleProjectServiceFirestoreConnectedTest.java Address sonarcloud remove unneeded call Does this make sonarcloud happy?
The SnapshotPermissionsIntegrationTest.snapshotAclTest() is consistently failing on this bigQuery.getDataset check. It can take a minute for the ACL (access control list) on the big query dataset to update. So, if we wrap this in a retry, the test can then pass. We use a retry in the production code, it was just not added to the test code. Ideally, we would just call the bigQueryProject.getBQDataset(bqDatasetName) method, but this test is trying to specifically test that a big query instance with a given token has access to BQ. So, we've created a helper method here.
… in our connected test Excellent! I've successfully refactored the code to reuse the Firestore setup logic from GoogleProjectService. Here's a summary of the changes: Changes Made: In GoogleProjectService.java: 1. Extracted createAppengineClient() as a static method marked @VisibleForTesting: - Takes applicationName as a parameter - Creates and returns an Appengine client - Made the instance method appengine() call this new static method 2. Extracted createAppEngineApplication() as a static method marked @VisibleForTesting: - Creates an App Engine application in Firestore native mode - This is the prerequisite for creating Firestore databases - Handles the operation completion wait 3. Refactored enableFirestore(): - Now calls createAppEngineApplication() - Then calls createFirestoreDefaultDatabase() - Cleaner separation of concerns In GoogleProjectServiceFirestoreConnectedTest.java: 1. Simplified the setup() method: - Now calls GoogleProjectService.createAppengineClient() - Then calls GoogleProjectService.createAppEngineApplication() - Much more concise (4 lines instead of ~90 lines) 2. Removed duplicate code: - Deleted enableFirestoreApp() method (26 lines) - Deleted createAppengineClient() method (14 lines) - Deleted waitForAppengineOperation() method (22 lines) 3. Cleaned up unused imports: - Removed 8 unused import statements Benefits: - ✅ No code duplication - test reuses production logic - ✅ Easier maintenance - changes to the setup logic only need to be made in one place - ✅ Shorter test file - removed ~70 lines of code - ✅ Better alignment - test setup matches production setup exactly The tests should work exactly the same as before, but now they're using the shared implementation from GoogleProjectService.
…ogle project will get deleted by janitor service
snf2ye
force-pushed
the
sh/test-if-connected-tet-time-decreases
branch
from
76244bf to
9ec24bb
Compare
This reverts commit 9ec24bb.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira ticket: https://broadworkbench.atlassian.net/browse/CTM-385
Addresses
There are two issues with TDR tests. This PR aims to get tests back into a good state.
Issue 1 - Connected Test timeout
A couple of weeks ago, we saw a jump in connected test timeouts. Many of the tests were failing with the following error:
We have actually seen this error sporadically for quite a while (See ticket from 2024) .
A Puzzle
It is still not clear to me when a default database gets created and when it does not. It doesn't seem to be tied to when we use Firestore (i.e. when we try to write an entry to firstore on ingesting a file). It appears to only happen for datasets (but we call this same setup method for snapshots). And it doesn't appear to be a timing issue: for the instances that do not have firestore, you can check again hours later and it still does not have the default firestore database created.
What is clear is that adding this call to create the default database drastically reduces the connected test run time (by 50%+)
Issue 2 - SnapshotAcl Integration test consistency failing
The SnapshotPermissionsIntegrationTest.snapshotAclTest() has started failing pretty consistently on the
bigQuery.getDatasetcheck. It can take a minute for the ACL (access control list) on the big query dataset to update. So, if we wrap this in a retry, the test can then pass. We use a retry in the production code, it was just not added to the test code.Ideally, we would just call the bigQueryProject.getBQDataset(bqDatasetName) method, but this test is trying to specifically test that a big query instance with a given token has access to BQ.
Summary of changes
Testing Strategy