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 3 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
10 changes: 9 additions & 1 deletion 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 @@ -31,6 +31,7 @@ const NO_WEB_TAG = 'no-web';
const NO_ANDROID_TAG = 'no-android';
const NO_IOS_TAG = 'no-ios';
const NO_LRU_TAG = 'no-lru';
const NO_MEMORY_PERSISTENCE = 'no-memory-persistence';
const BENCHMARK_TAG = 'benchmark';
const KNOWN_TAGS = [
BENCHMARK_TAG,
Expand All @@ -39,7 +40,8 @@ const KNOWN_TAGS = [
NO_WEB_TAG,
NO_ANDROID_TAG,
NO_IOS_TAG,
NO_LRU_TAG
NO_LRU_TAG,
NO_MEMORY_PERSISTENCE
];

// TOOD(mrschmidt): Make this configurable with mocha options.
Expand Down Expand Up @@ -78,6 +80,12 @@ 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_MEMORY_PERSISTENCE) !== -1
Copy link
Contributor

Choose a reason for hiding this comment

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

We should avoid these inversely-phrased tags. They're hard to reason about because they don't state what's required, but rather what's excluded.

This one could be named something like durable-persistence, which would make it more obvious to the reader that this is the property of the persistence layer that's interesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

We used to have a tag that was called just 'PERSISTENCE', which might imply that it is durable.

Copy link
Author

Choose a reason for hiding this comment

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

I agree with the general principle, but I did it this way to match how the rest of the tags were implemented. Do you think it's worth deviating?

Copy link
Contributor

@wilhuff wilhuff Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

benchmark, exclusive, and multiclient follow the positive form. I think we should continue that.

no-web, no-ios, and no-android are a mistake; I think they should have been phrased the other way too, especially since we tend to add a feature on a new platform, so it's easier to phrase this in terms of which platform is going first.

That leaves no-lru, which I think we could stand to be positive, but I don't know what the positive form would be.

In any case, I think positive phrasing could be a long term goal and this would be a step in that direction.

Also, since you're going to have to edit every runner to implement this, you could just refactor all to positive.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'll make the change.

Copy link
Author

Choose a reason for hiding this comment

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

Switched the tags to be positive.

Copy link
Author

Choose a reason for hiding this comment

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

Switch the platform tags back to negative.

) {
// Test requires actual persistence, but it's not enabled. Skip it.
return it.skip;
} else if (persistenceEnabled && tags.indexOf(NO_LRU_TAG) !== -1) {
// spec should have a comment explaining why it is being skipped.
return it.skip;
Expand Down
50 changes: 27 additions & 23 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 @@ -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)',
[],
['no-memory-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',
['no-memory-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',
[],
['no-memory-persistence'],
() => {
const initialVersion = 1000;
const minutesLater = 5 * 60 * 1e6 + initialVersion;
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',
['no-memory-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',
['no-memory-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', ['no-memory-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)',
[],
['no-memory-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