Skip to content

Fix Spec tests 'Limbo documents stay consistent' #1994

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.

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
Jul 17, 2019

Conversation

schmidt-sebastian
Copy link
Contributor

The 'Limbo documents stay consistent' used the multi-client runner instead of the spec-runner. This PR fixes the test and adds an assert to make sure that this doesn't happen again.

@schmidt-sebastian
Copy link
Contributor Author

Assigning this to @mikelehen since he is back and its more of his area of expertise :) You are welcome!

step.clientIndex === undefined || tags.indexOf(MULTI_CLIENT_TAG) !== -1,
"Cannot use 'client()' to initialize a test that is not tagged with " +
"'multi-client'. Did you mean to use 'spec()'?"
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it'd make sense to assert the inverse as well (if this is a multi-client test, then clientIndex must be defined)? Basically:

const isMultiClientTest = tags.indexOf(MULTI_CLIENT_TAG) !== -1;
const hasClientIndex = step.clientIndex !== undefined;
assert(isMultiClientTest === hasClientIndex, "Tests can use client() if-and-only-if they are tagged with 'multi-client'.");

I don't care strongly if that seems pointless though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not pointless, but not necessarily worth opening an IDE for and fixing since it would not break anything. I'll leave it as an imaginary TODO :)


const docA = doc('collection/a', 1000, { matches: true });
const docADirty = doc(
'collection/a',
1000,
{ matches: true },
{ matches: true },
Copy link
Contributor

Choose a reason for hiding this comment

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

Hrm. Strange that prettier allowed an extra space before...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't. But the change was never pushed.

@schmidt-sebastian schmidt-sebastian merged commit 22caf9f into master Jul 17, 2019
@mikelehen mikelehen deleted the mrschmidt/specfix branch July 17, 2019 00:46
@firebase firebase locked and limited conversation to collaborators Oct 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants