Skip to content

Commit

Permalink
Fix composite index testing build bug (#7682)
Browse files Browse the repository at this point in the history
  • Loading branch information
milaGGL authored Oct 13, 2023
1 parent d2e4774 commit 8283a55
Show file tree
Hide file tree
Showing 9 changed files with 41 additions and 19 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/test-changed-firestore-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ jobs:
if: github.event_name == 'pull_request'
run: |
cd packages/firestore
terraform apply -var-file=../../config/project.json -auto-approve
terraform apply -var-file=../../config/project.json -auto-approve &> /dev/null
continue-on-error: true
- name: Set up Node (16)
uses: actions/setup-node@v3
Expand Down
2 changes: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -97,4 +97,4 @@ toc/
.terraform/*
.terraform.lock.hcl
*.tfstate
*.tfstate.*
*.tfstate.*
1 change: 1 addition & 0 deletions integration/firestore/gulpfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ function copyTests() {
.src(
[
testBase + '/integration/api/*.ts',
testBase + '/integration/util/composite_index_test_helper.ts',
testBase + '/integration/util/events_accumulator.ts',
testBase + '/integration/util/helpers.ts',
testBase + '/integration/util/settings.ts',
Expand Down
1 change: 1 addition & 0 deletions packages/firestore/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,7 @@ export { FieldPath as _FieldPath } from './model/path';
export type { ResourcePath as _ResourcePath } from './model/path';
export { ByteString as _ByteString } from './util/byte_string';
export { logWarn as _logWarn } from './util/log';
export { AutoId as _AutoId } from './util/misc';
export type {
AuthTokenFactory,
FirstPartyCredentialsSettings
Expand Down
6 changes: 6 additions & 0 deletions packages/firestore/src/util/misc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@ export interface Indexable {
[k: string]: unknown;
}

/**
* A utility class for generating unique alphanumeric IDs of a specified length.
*
* @internal
* Exported internally for testing purposes.
*/
export class AutoId {
static newId(): string {
// Alphanumeric characters
Expand Down
18 changes: 18 additions & 0 deletions packages/firestore/test/integration/api/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,21 @@ put in test/integration/api_internal instead.
The line "import * as firebaseExport from '../util/firebase_export';" is
replaced via the gulpfile in 'integration/firestore' and should not be
modified.


## Testing composite index query against production

### Setting Up the Environment:
1. Create a `project.json` file in the `firebase-js-sdk/config` directory. This file should contain your target Firebase project's configuration.
2. If not already logged in, authenticate with your Google Cloud Platform (GCP) account using `gcloud auth application-default login`. You can check your logged-in accounts by running `gcloud auth list`.
3. Navigate to the `firebase-js-sdk/packages/firestore` directory, run:
```
terraform init
terraform apply -var-file=../../config/project.json -auto-approve
```
Note: If the index creation encounters issues, such as concurrent operations, consider running the index creation process again. Error messages indicating that indexes have already been created can be safely disregarded.


### Adding new composite index query tests
1. To create a new composite index for local development, click on the provided link in the test error message, which will direct you to the Firebase Console.
2. Add the newly created composite index to the `firestore_index_config.tf` file. The "__name__" field is not required to be explicitly added to the file, as the index creation will auto complete it on behalf.
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,10 @@ import { apiDescribe } from '../util/helpers';
* and setting test documents and running queries with ease, ensuring proper data
* isolation and query construction.
*
* Please remember to update the main index configuration file (firestore_index_config.tf)
* with any new composite indexes needed for the tests. This ensures synchronization with
* other testing environments, including CI. You can generate the required index link by
* clicking on the Firebase console link in the error message while running tests locally.
* To get started, please refer to the instructions provided in the README file. This will guide you
* through setting up your local testing environment and updating the Terraform configuration with
* any new composite indexes required for your testing scenarios.
*/

apiDescribe('Composite Index Queries', persistence => {
// OR Query tests only run when the SDK's local cache is configured to use
// LRU garbage collection (rather than eager garbage collection) because
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,6 @@
* limitations under the License.
*/

import { and } from '../../../src/lite-api/query';
import { AutoId } from '../../../src/util/misc';
import { field } from '../../util/helpers';

import {
query as internalQuery,
CollectionReference,
Expand All @@ -41,7 +37,10 @@ import {
getDocs as getDocuments,
QuerySnapshot,
deleteDoc as deleteDocument,
doc
doc,
and,
_AutoId,
_FieldPath
} from './firebase_export';
import {
batchCommitDocsToCollection,
Expand Down Expand Up @@ -72,7 +71,7 @@ export class CompositeIndexTestHelper {
// Creates a new instance of the CompositeIndexTestHelper class, with a unique test
// identifier for data isolation.
constructor() {
this.testId = 'test-id-' + AutoId.newId();
this.testId = 'test-id-' + _AutoId.newId();
}

// Runs a test with specified documents in the COMPOSITE_INDEX_TEST_COLLECTION.
Expand Down Expand Up @@ -113,8 +112,8 @@ export class CompositeIndexTestHelper {

// Remove test-specific fields from a document, including the testId and expiration date.
private removeTestSpecificFieldsFromDoc(doc: DocumentData): void {
doc._document?.data?.delete(field(this.TTL_FIELD));
doc._document?.data?.delete(field(this.TEST_ID_FIELD));
doc._document?.data?.delete(new _FieldPath([this.TEST_ID_FIELD]));
doc._document?.data?.delete(new _FieldPath([this.TEST_ID_FIELD]));
}

// Helper method to hash document keys and add test-specific fields for the provided documents.
Expand Down
7 changes: 3 additions & 4 deletions packages/firestore/test/integration/util/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
import { isIndexedDBAvailable } from '@firebase/util';
import { expect } from 'chai';

import { AutoId } from '../../../src/util/misc';

import {
clearIndexedDbPersistence,
collection,
Expand All @@ -45,7 +43,8 @@ import {
writeBatch,
Query,
getDocsFromServer,
getDocsFromCache
getDocsFromCache,
_AutoId
} from './firebase_export';
import {
ALT_PROJECT_ID,
Expand Down Expand Up @@ -448,7 +447,7 @@ export function withTestCollectionSettings<T>(
docs: { [key: string]: DocumentData },
fn: (collection: CollectionReference, db: Firestore) => Promise<T>
): Promise<T> {
const collectionId = AutoId.newId();
const collectionId = _AutoId.newId();
return batchCommitDocsToCollection(
persistence,
settings,
Expand Down

0 comments on commit 8283a55

Please sign in to comment.