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

Migrate deprecated match query syntax #11554

Merged
merged 6 commits into from May 5, 2017
Jump to file or symbol
Failed to load files and symbols.
+1 −0
Diff settings

Always

Just for now

Viewing a subset of changes. View all
Prev

tighten up test

  • Loading branch information...
Bargs committed May 5, 2017
commit 53edcb818a2efe2872a7feb137b816f72427ca85
@@ -39,6 +39,7 @@ describe('migrateFilter', function () {
};
const migratedFilter = migrateFilter(originalFilter);
expect(migratedFilter).to.be(originalFilter);

This comment has been minimized.

@lukasolson

lukasolson May 4, 2017

Member

Maybe you could add to this test

expect(migratedFilter).to.not.eql(newMatchPhraseFilter);

Just to make sure that the filter hasn't been modified.

This comment has been minimized.

@Bargs

Bargs May 4, 2017

Contributor

Good point, would the following be better though?

expect(_.isEqual(migratedFilter, originalFilter)).to.be(true);

This comment has been minimized.

@lukasolson

lukasolson May 4, 2017

Member

Either way is really fine... I prefer to.eql because when you use the syntax you've suggested, then the error is reported as something like "expected false to be true" instead of "expected { foo: bar } to sort of equal { foo: baz }" or whatever.

This comment has been minimized.

@Bargs

Bargs May 4, 2017

Contributor

I talking more about the idea of checking the equality between migratedFilter and originalFilter instead of checking that migratedFilter is not eql to newMatchPhraseFilter. It seems that the former would catch more errors, but I wasn't sure if I was missing something about what you were suggesting.

As an aside, I agree that the error reporting sucks when using _.isEqual but the problem I have with expect.js's eql function is that it uses loose equality (==) so there are subtle bugs it would miss. Another reason I wish we were using Chai.

This comment has been minimized.

@Bargs
expect(_.isEqual(migratedFilter, originalFilter)).to.be(true);
});
});
ProTip! Use n and p to navigate between commits in a pull request.