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

Expect inequality #10

Closed
sol opened this issue Jul 1, 2014 · 18 comments
Closed

Expect inequality #10

sol opened this issue Jul 1, 2014 · 18 comments

Comments

@sol
Copy link
Member

sol commented Jul 1, 2014

Currently, if you want to state that two things are different, you can use something like:

23 `shouldSatisfy` (not . (== 42))

Do we want to have a separate combinator for that, so that you can say:

23 `shouldNotBe` 42

If yes, should we also add shouldNotContain, shouldNotSatisfy and shouldNotReturn?

@sol
Copy link
Member Author

sol commented Jul 1, 2014

Originally motivated by hspec/hspec#174.

We have a PR that adds shouldNotBe, shouldNotContain, shouldNotSatisfy and shouldNotReturn (#8, thanks @Jefffrey).

@erikd
Copy link

erikd commented Jul 1, 2014

+1 from me.

@mwotton
Copy link

mwotton commented Jul 1, 2014

i'm a bit torn - it would be convenient, but it does sort of encourage bad tests.

@sol
Copy link
Member Author

sol commented Jul 1, 2014

@mwotton I think this is my main concern.

I'm tempted to just add shouldNotSatisfy, which simplifies the above example to

23 `shouldNotSatisfy` (== 42)

and can be useful in other context, e.g.

xs `shouldNotSatisfy` null

@mwotton
Copy link

mwotton commented Jul 1, 2014

+1, i like shouldNotSatisfy

@shoooe
Copy link

shoooe commented Jul 1, 2014

Compare:

Matrix 
    [1, 2, 3
    , 4, 5, 6
    , 7, 8, 8]
    `shouldNotBe`
    Matrix
        [1, 2, 3
        , 4, 5, 6
        , 7, 8, 9]

to:

Matrix 
    [1, 2, 3
    , 4, 5, 6
    , 7, 8, 8]
    `shouldSatisfy`
        (/=
            Matrix
                [1, 2, 3
                , 4, 5, 6
                , 7, 8, 9] )

shouldNotBe is easier to read, creates symmetry (with all the other functions provided) and doesn't change a single thing about the library itself.

I really don't see the problems and I can see some benefits.

@fmap
Copy link

fmap commented Jul 1, 2014

Do you know what a double negative is?

@sol
Copy link
Member Author

sol commented Jul 1, 2014

@Jefffrey You can change

`shouldNotSatisfy` (not . (== ...))

to

`shouldSatisfy` (/= ..)

@mwotton
Copy link

mwotton commented Jul 2, 2014

It makes it easier to write something you probably shouldn't write 99% of
the time - a test that rules out one possible result out of possible
millions is not as good as one that rules out all but one. Sometimes it's
unavoidable, but it probably shouldn't be encouraged.

On Tue, Jul 1, 2014 at 7:34 PM, Jeffrey Pigarelli notifications@github.com
wrote:

@fmap https://github.com/fmap @sol https://github.com/sol Yes, I
miswrote that. Now, let's see: what are the cons to adding these 4/5
functions?


Reply to this email directly or view it on GitHub
#10 (comment)
.

A UNIX signature isn't a return address, it's the ASCII equivalent of a
black velvet clown painting. It's a rectangle of carets surrounding a
quote from a literary giant of weeniedom like Heinlein or Dr. Who.
-- Chris Maeda

@sol
Copy link
Member Author

sol commented Jul 2, 2014

@mwotton + the test that "rules out all but one" is much better as documentation.

@mwotton
Copy link

mwotton commented Jul 2, 2014

actually, only one of those was written after your example. You might want to steady on with the wild accusations of hubris.

as far as your test case goes, a QC property that shows that changing any given cell invalidates the equality of a matrix would be infinitely better anyway.

@mwotton
Copy link

mwotton commented Jul 2, 2014

Sure, but this is a Haskell testing lib. We're not bound by the lowest common denominator of languages.

Not sure what you mean by your second point. That is testing for inequality, no? (I assumed you were talking about some clever manual Eq instance rather than automatically derived ones, otherwise yes, it would be a bit pointless.)

@mwotton
Copy link

mwotton commented Jul 2, 2014

I think it's fine in the context of a QC property, yeah.

@mwotton
Copy link

mwotton commented Jul 3, 2014

I think you may have missed where the burden of proof lies here. You're proposing a change: you need to find an example where there isn't an unambiguously better alternative. You proposed an example, I showed a better alternative.

@mooreniemi
Copy link

good discussion here. the only input i'd add is that for UI tests, ime, shouldNot can be helpful. that type of matcher is in jasmine-jquery, for instance.

@sol
Copy link
Member Author

sol commented Jul 8, 2014

People, sorry for sitting on that patch. I'll make an educated decision as soon as I have some time to spend on this.

@sol
Copy link
Member Author

sol commented Jul 9, 2014

My final opinion on this, let's add them to Test.Hspec.Expectations.Contrib, if we see that they are widely used, we can always re-export them from Test.Hspec.Expectations later.

@Jefffrey Can you update your PR?

@sol
Copy link
Member Author

sol commented Aug 11, 2014

Solved by #11.

@sol sol closed this as completed Aug 11, 2014
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

No branches or pull requests

6 participants