Skip to content

Stop saving persistence layer across spec tests #1129

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 7 commits into from
Aug 17, 2018
Merged

Stop saving persistence layer across spec tests #1129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
611b778
Add no-memory-persistence flag to spec tests.
Aug 15, 2018
84d810c
Restart persistence for spec tests
Aug 16, 2018
8e06033
[AUTOMATED]: Prettier Code Styling
Aug 16, 2018
bdb8a54
No more negative tags
Aug 17, 2018
9ad2d4e
[AUTOMATED]: Prettier Code Styling
Aug 17, 2018
fa8bcbe
Put back the no-platform tags
Aug 17, 2018
f3b5702
[AUTOMATED]: Prettier Code Styling
Aug 17, 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
19 changes: 14 additions & 5 deletions packages/firestore/test/unit/specs/describe_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,15 @@ import { SpecStep } from './spec_test_runner';
// Disables all other tests; useful for debugging. Multiple tests can have
// this tag and they'll all be run (but all others won't).
const EXCLUSIVE_TAG = 'exclusive';
// Multi-client related tests (which imply persistence).
const MULTI_CLIENT_TAG = 'multi-client';
// Explicit per-platform disable flags.
const NO_WEB_TAG = 'no-web';
const NO_ANDROID_TAG = 'no-android';
const NO_IOS_TAG = 'no-ios';
const NO_LRU_TAG = 'no-lru';
// The remaining tags specify features that must be present to run a given test
// Multi-client related tests (which imply persistence).
const MULTI_CLIENT_TAG = 'multi-client';
const EAGER_GC_TAG = 'eager-gc';
const DURABLE_PERSISTENCE_TAG = 'durable-persistence';
const BENCHMARK_TAG = 'benchmark';
const KNOWN_TAGS = [
BENCHMARK_TAG,
Expand All @@ -39,7 +41,8 @@ const KNOWN_TAGS = [
NO_WEB_TAG,
NO_ANDROID_TAG,
NO_IOS_TAG,
NO_LRU_TAG
EAGER_GC_TAG,
DURABLE_PERSISTENCE_TAG
];

// TOOD(mrschmidt): Make this configurable with mocha options.
Expand Down Expand Up @@ -78,7 +81,13 @@ export function setSpecJSONHandler(writer: (json: string) => void): void {
function getTestRunner(tags, persistenceEnabled): Function {
if (tags.indexOf(NO_WEB_TAG) >= 0) {
return it.skip;
} else if (persistenceEnabled && tags.indexOf(NO_LRU_TAG) !== -1) {
} else if (
!persistenceEnabled &&
tags.indexOf(DURABLE_PERSISTENCE_TAG) !== -1
) {
// Test requires actual persistence, but it's not enabled. Skip it.
return it.skip;
} else if (persistenceEnabled && tags.indexOf(EAGER_GC_TAG) !== -1) {
// spec should have a comment explaining why it is being skipped.
return it.skip;
} else if (!persistenceEnabled && tags.indexOf(MULTI_CLIENT_TAG) !== -1) {
Expand Down
52 changes: 28 additions & 24 deletions packages/firestore/test/unit/specs/listen_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ describeSpec('Listens:', [], () => {
// Obviously this test won't hold with offline persistence enabled.
specTest(
'Contents of query are cleared when listen is removed.',
['no-lru'],
['eager-gc'],
'Explicitly tests eager GC behavior',
() => {
const query = Query.atPath(path('collection'));
Expand Down Expand Up @@ -228,7 +228,7 @@ describeSpec('Listens:', [], () => {
// This would only happen when we use a resume token, but omitted for brevity.
specTest(
'Will gracefully handle watch stream reverting snapshots (with restart)',
[],
['durable-persistence'],
() => {
const query = Query.atPath(path('collection'));
const docAv1 = doc('collection/a', 1000, { v: 'v1000' });
Expand Down Expand Up @@ -570,34 +570,38 @@ describeSpec('Listens:', [], () => {
);
});

specTest('Omits global resume tokens for a short while', [], () => {
const query = Query.atPath(path('collection'));
const docA = doc('collection/a', 1000, { key: 'a' });
specTest(
'Omits global resume tokens for a short while',
['durable-persistence'],
() => {
const query = Query.atPath(path('collection'));
const docA = doc('collection/a', 1000, { key: 'a' });

return (
spec()
.withGCEnabled(false)
.userListens(query)
.watchAcksFull(query, 1000, docA)
.expectEvents(query, { added: [docA] })
return (
spec()
.withGCEnabled(false)
.userListens(query)
.watchAcksFull(query, 1000, docA)
.expectEvents(query, { added: [docA] })

// One millisecond later, watch sends an updated resume token but the
// user doesn't manage to unlisten before restart.
.watchSnapshots(2000, [], 'resume-token-2000')
.restart()
// One millisecond later, watch sends an updated resume token but the
// user doesn't manage to unlisten before restart.
.watchSnapshots(2000, [], 'resume-token-2000')
.restart()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make restart check that the tag is set? Are there tests that have any business restarting without durable persistence?

Copy link
Author

Choose a reason for hiding this comment

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

We could make restart check, if we add something like abstract isDurable(): boolean to TestRunner. While implementing this I had an assert for instanceof MemoryPersistence, but decided that was too hacky to check in. I'll add it if you think it's worth it.

There are not currently tests that make sense with non-durable persistence and a restart, but that's also because we couldn't really write them before. One could now write a test for memory persistence behavior across a restart.

Copy link
Author

Choose a reason for hiding this comment

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

I left out this feature. We can add it if we need it.


.userListens(query, 'resume-token-1000')
.expectEvents(query, { added: [docA], fromCache: true })
.watchAcks(query)
.watchCurrents(query, 'resume-token-3000')
.watchSnapshots(3000)
.expectEvents(query, { fromCache: false })
);
});
.userListens(query, 'resume-token-1000')
.expectEvents(query, { added: [docA], fromCache: true })
.watchAcks(query)
.watchCurrents(query, 'resume-token-3000')
.watchSnapshots(3000)
.expectEvents(query, { fromCache: false })
);
}
);

specTest(
'Persists global resume tokens if the snapshot is old enough',
[],
['durable-persistence'],
() => {
const initialVersion = 1000;
const minutesLater = 5 * 60 * 1e6 + initialVersion;
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/specs/offline_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describeSpec('Offline:', [], () => {

specTest(
'Removing all listeners delays "Offline" status on next listen',
['no-lru'],
['eager-gc'],
'Marked as no-lru because when a listen is re-added, it gets a new target id rather than reusing one',
() => {
const query = Query.atPath(path('collection'));
Expand Down
86 changes: 47 additions & 39 deletions packages/firestore/test/unit/specs/persistence_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,48 +22,56 @@ import { client, spec } from './spec_builder';
import { TimerId } from '../../../src/util/async_queue';

describeSpec('Persistence:', [], () => {
specTest('Local mutations are persisted and re-sent', [], () => {
return spec()
.userSets('collection/key1', { foo: 'bar' })
.userSets('collection/key2', { baz: 'quu' })
.restart()
.expectNumOutstandingWrites(2)
.writeAcks('collection/key1', 1, { expectUserCallback: false })
.writeAcks('collection/key2', 2, { expectUserCallback: false })
.expectNumOutstandingWrites(0);
});

specTest('Persisted local mutations are visible to listeners', [], () => {
const query = Query.atPath(path('collection'));
return (
spec()
specTest(
'Local mutations are persisted and re-sent',
['durable-persistence'],
() => {
return spec()
.userSets('collection/key1', { foo: 'bar' })
.userSets('collection/key2', { baz: 'quu' })
.restart()
// It should be visible to listens.
.userListens(query)
.expectEvents(query, {
added: [
doc(
'collection/key1',
0,
{ foo: 'bar' },
{ hasLocalMutations: true }
),
doc(
'collection/key2',
0,
{ baz: 'quu' },
{ hasLocalMutations: true }
)
],
fromCache: true,
hasPendingWrites: true
})
);
});
.expectNumOutstandingWrites(2)
.writeAcks('collection/key1', 1, { expectUserCallback: false })
.writeAcks('collection/key2', 2, { expectUserCallback: false })
.expectNumOutstandingWrites(0);
}
);

specTest(
'Persisted local mutations are visible to listeners',
['durable-persistence'],
() => {
const query = Query.atPath(path('collection'));
return (
spec()
.userSets('collection/key1', { foo: 'bar' })
.userSets('collection/key2', { baz: 'quu' })
.restart()
// It should be visible to listens.
.userListens(query)
.expectEvents(query, {
added: [
doc(
'collection/key1',
0,
{ foo: 'bar' },
{ hasLocalMutations: true }
),
doc(
'collection/key2',
0,
{ baz: 'quu' },
{ hasLocalMutations: true }
)
],
fromCache: true,
hasPendingWrites: true
})
);
}
);

specTest('Remote documents are persisted', [], () => {
specTest('Remote documents are persisted', ['durable-persistence'], () => {
const query = Query.atPath(path('collection'));
const doc1 = doc('collection/key', 1000, { foo: 'bar' });
return spec()
Expand Down Expand Up @@ -126,7 +134,7 @@ describeSpec('Persistence:', [], () => {

specTest(
'Mutation Queue is persisted across uid switches (with restarts)',
[],
['durable-persistence'],
() => {
return spec()
.userSets('users/anon', { uid: 'anon' })
Expand Down
9 changes: 3 additions & 6 deletions packages/firestore/test/unit/specs/spec_test_runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,10 +418,6 @@ abstract class TestRunner {

async start(): Promise<void> {
this.persistence = await this.initPersistence(this.serializer);
await this.init();
}

private async init(): Promise<void> {
const garbageCollector = this.getGarbageCollector();

this.sharedClientState = this.getSharedClientState();
Expand Down Expand Up @@ -872,13 +868,14 @@ abstract class TestRunner {
}

private async doRestart(): Promise<void> {
// Reinitialize everything, except the persistence.
// Reinitialize everything.
// No local store to shutdown.
await this.remoteStore.shutdown();
await this.persistence.shutdown(/* deleteData= */ false);

// We have to schedule the starts, otherwise we could end up with
// interleaved events.
await this.queue.enqueue(() => this.init());
await this.queue.enqueue(() => this.start());
}

private async doApplyClientState(state: SpecClientState): Promise<void> {
Expand Down
2 changes: 1 addition & 1 deletion packages/firestore/test/unit/specs/write_spec.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,7 @@ describeSpec('Writes:', [], () => {

specTest(
'Held writes are released when there are no queries left.',
['no-lru'],
['eager-gc'],
'This test expects a new target id for a new listen, but without eager gc, the same target id is reused',
() => {
const query = Query.atPath(path('collection'));
Expand Down