Skip to content

Move isShutdown to state expectations #2102

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 2 commits into from
Aug 21, 2019
+11 −16
Merged

Move isShutdown to state expectations #2102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa9cd56
Move isShutdown to state expectations
schmidt-sebastian Aug 21, 2019
8244160
Fix unit test
schmidt-sebastian Aug 21, 2019
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
4 changes: 2 additions & 2 deletions packages/firestore/test/unit/local/persistence_test_helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,8 @@ export async function testMemoryLruPersistence(
}

/** Clears the persistence in tests */
export async function clearTestPersistence(): Promise<void> {
await IndexedDbPersistence.clearPersistence(TEST_PERSISTENCE_PREFIX);
export function clearTestPersistence(): Promise<void> {
return IndexedDbPersistence.clearPersistence(TEST_PERSISTENCE_PREFIX);
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is a quiz on how async/await works, but I'm stumped... Does your change do anything?

Copy link
Contributor Author

@schmidt-sebastian schmidt-sebastian Aug 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

No... but I find it weird to use async in a method that only uses a single Promise in its return statement. I can turn it into a quiz though!

}

class NoOpSharedClientStateSyncer implements SharedClientStateSyncer {
Expand Down
8 changes: 4 additions & 4 deletions packages/firestore/test/unit/specs/spec_builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -395,10 +395,10 @@ export class SpecBuilder {
}

expectIsShutdown(): this {
this.nextStep();
this.currentStep = {
expectIsShutdown: true
};
this.assertStep('Active target expectation requires previous step');
const currentStep = this.currentStep!;
currentStep.stateExpect = currentStep.stateExpect || {};
currentStep.stateExpect.isShutdown = true;
return this;
}

Expand Down
15 changes: 5 additions & 10 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 @@ -569,8 +569,6 @@ abstract class TestRunner {
return this.doRestart();
} else if ('shutdown' in step) {
return this.doShutdown();
} else if ('expectIsShutdown' in step) {
return this.doExpectIsShutdown();
} else if ('applyClientState' in step) {
// PORTING NOTE: Only used by web multi-tab tests.
return this.doApplyClientState(step.applyClientState!);
Expand Down Expand Up @@ -885,11 +883,6 @@ abstract class TestRunner {
this.started = false;
}

/** Asserts that the client is shutdown by */
private async doExpectIsShutdown(): Promise<void> {
expect(this.started).to.equal(false);
}

private async doClearPersistence(): Promise<void> {
await clearTestPersistence();
}
Expand Down Expand Up @@ -985,6 +978,9 @@ abstract class TestRunner {
if ('isPrimary' in expectation) {
expect(this.isPrimaryClient).to.eq(expectation.isPrimary!, 'isPrimary');
}
if ('isShutdown' in expectation) {
expect(this.started).to.equal(!expectation.isShutdown);
}
}

if (expectation && expectation.userCallbacks) {
Expand Down Expand Up @@ -1396,9 +1392,6 @@ export interface SpecStep {
/** Shut down the client and close it network connection. */
shutdown?: true;

/** Assert that the firestore client is shutdown. */
expectIsShutdown?: true;

/**
* Optional list of expected events.
* If not provided, the test will fail if the step causes events to be raised.
Expand Down Expand Up @@ -1577,6 +1570,8 @@ export interface StateExpectation {
* Whether the instance holds the primary lease. Used in multi-client tests.
*/
isPrimary?: boolean;
/** Whether the client is shutdown. */
isShutdown?: boolean;
/**
* Current expected active targets. Verified in each step until overwritten.
*/
Expand Down