Skip to content

Speed up FieldPath validation tests #1147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Aug 21, 2018
Merged

Speed up FieldPath validation tests #1147

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5943bed
Speed up FieldPath validation tests
schmidt-sebastian Aug 20, 2018
6b50ac5
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Aug 20, 2018
eeb5fd5
Lint fix
schmidt-sebastian Aug 20, 2018
432f1fe
[AUTOMATED]: Prettier Code Styling
schmidt-sebastian Aug 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
116 changes: 64 additions & 52 deletions packages/firestore/test/integration/api/validation.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -560,37 +560,53 @@ apiDescribe('Validation:', persistence => {
);

validationIt(persistence, 'Field paths must not have empty segments', db => {
const badFieldPaths = ['', 'foo..baz', '.foo', 'foo.'];
const promises = [];
for (const fieldPath of badFieldPaths) {
const reason =
`Invalid field path (${fieldPath}). Paths must not be ` +
`empty, begin with '.', end with '.', or contain '..'`;
promises.push(expectFieldPathToFail(db, fieldPath, reason));
}
return Promise.all(promises);
const docRef = db.collection('test').doc();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Optional nit: consider refactoring the piece of code that promises a snapshot into a helper function. OTOH, since it's only repeated twice, it might not be worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You had me at "optional". I'll leave it as is for now.

return docRef
.set({ test: 1 })
.then(() => {
return docRef.get();
})
.then(snapshot => {
const badFieldPaths = ['', 'foo..baz', '.foo', 'foo.'];
const promises = [];
for (const fieldPath of badFieldPaths) {
const reason =
`Invalid field path (${fieldPath}). Paths must not be ` +
`empty, begin with '.', end with '.', or contain '..'`;
promises.push(expectFieldPathToFail(snapshot, fieldPath, reason));
}
return Promise.all(promises);
});
});

validationIt(
persistence,
'Field paths must not have invalid segments',
db => {
const badFieldPaths = [
'foo~bar',
'foo*bar',
'foo/bar',
'foo[1',
'foo]1',
'foo[1]'
];
const promises = [];
for (const fieldPath of badFieldPaths) {
const reason =
`Invalid field path (${fieldPath}). Paths must not ` +
`contain '~', '*', '/', '[', or ']'`;
promises.push(expectFieldPathToFail(db, fieldPath, reason));
}
return Promise.all(promises);
const docRef = db.collection('test').doc();
return docRef
.set({ test: 1 })
.then(() => {
return docRef.get();
})
.then(snapshot => {
const badFieldPaths = [
'foo~bar',
'foo*bar',
'foo/bar',
'foo[1',
'foo]1',
'foo[1]'
];
const promises = [];
for (const fieldPath of badFieldPaths) {
const reason =
`Invalid field path (${fieldPath}). Paths must not ` +
`contain '~', '*', '/', '[', or ']'`;
promises.push(expectFieldPathToFail(snapshot, fieldPath, reason));
}
return Promise.all(promises);
});
}
);

Expand Down Expand Up @@ -953,37 +969,33 @@ function expectWriteToFail(
* they fail with the specified reason.
*/
function expectFieldPathToFail(
db: firestore.FirebaseFirestore,
snapshot: firestore.DocumentSnapshot,
path: string,
reason: string
): Promise<void> {
// Get an arbitrary snapshot we can use for testing.
const docRef = db.collection('test').doc();
return docRef
.set({ test: 1 })
.then(() => {
return docRef.get();
})
.then(snapshot => {
// Snapshot paths.
expect(() => snapshot.get(path)).to.throw(
'Function DocumentSnapshot.get() called with invalid data. ' + reason
);
return Promise.resolve().then(() => {
// Snapshot paths.
expect(() => snapshot.get(path)).to.throw(
'Function DocumentSnapshot.get() called with invalid data. ' + reason
);

// Query filter / order fields.
const coll = db.collection('test-collection');
// <=, etc omitted for brevity since the code path is trivially
// shared.
expect(() => coll.where(path, '==', 1)).to.throw(
'Function Query.where() called with invalid data. ' + reason
);
expect(() => coll.orderBy(path)).to.throw(
'Function Query.orderBy() called with invalid data. ' + reason
);
const db = snapshot.ref.firestore;

// Update paths.
const data = {} as { [field: string]: number };
data[path] = 1;
return expectUpdateToFail(db, data, reason);
});
// Query filter / order fields.
const coll = db.collection('test-collection');
// <=, etc omitted for brevity since the code path is trivially
// shared.
expect(() => coll.where(path, '==', 1)).to.throw(
'Function Query.where() called with invalid data. ' + reason
);
expect(() => coll.orderBy(path)).to.throw(
'Function Query.orderBy() called with invalid data. ' + reason
);

// Update paths.
const data = {} as { [field: string]: number };
data[path] = 1;
return expectUpdateToFail(db, data, reason);
});
}