Skip to content
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

Create specs for $lt and $gt (among others) comparators #366

Merged
merged 4 commits into from
Mar 8, 2016
Merged

Create specs for $lt and $gt (among others) comparators #366

merged 4 commits into from
Mar 8, 2016

Conversation

alexandernst
Copy link
Collaborator

No description provided.

@alexandernst
Copy link
Collaborator Author

@obeliskos Please note that in order to test the comparators without making funny things with the code, I exposed LokiOps. It shouldn't matter, but I thought I'd mention it just in case.

Also note that the specs won't pass because gt and lt comparators are still buggy. Tests should be fixed after merging #365

@alexandernst
Copy link
Collaborator Author

@obeliskos Some "crazy" tests are missing. "Crazy" as in "comparing anything with anything".
I'm not sure what should those return. For example true > 'foobar'? new Date(2015) > 42?
Should those just return whatever the JS interpreter returns?

@techfort
Copy link
Owner

@alexandernst @obeliskos if i can interject, if you compare a boolean to a string i'd throw a new Error('change jobs, dude.') . Having said that, we can assume people know what they are doing so the result should be - as @alexandernst said - what the interpreter returns (wilth all the caveats of JS, such as {} + [] being 0, but [] + [] being '' and so on...)

@alexandernst
Copy link
Collaborator Author

I'm almost sure the current code (not this specs, but the comparators) covers all possible cases of "crazy" combinations. And when I say "almost" I actually mean "I haven't found a combination that doesn't return what it should".

Are we good with this PR? Remember that this mist be merge only after #365

@alexandernst
Copy link
Collaborator Author

Ok, now that #365 got merged, I'll restart travis here

@alexandernst alexandernst reopened this Mar 8, 2016
@obeliskos
Copy link
Collaborator

Whew, thanks for these tests (and fixes).

obeliskos added a commit that referenced this pull request Mar 8, 2016
Create specs for $lt and $gt (among others) comparators
@obeliskos obeliskos merged commit 3d2cf95 into techfort:master Mar 8, 2016
@alexandernst
Copy link
Collaborator Author

No problem :)

@alexandernst alexandernst mentioned this pull request Mar 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants