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

Proposal: .either.some.or.someElse #620

Closed
zxqfox opened this Issue Feb 12, 2016 · 12 comments

Comments

Projects
None yet
7 participants
@zxqfox
Copy link

zxqfox commented Feb 12, 2016

Would be nice to have .either as a delimiter and .or as finalizer of assertion chain.

Something like:

.to.be.equal('five').either.an('object').or.a('number')

There could be just one .or. And as many as need .eithers.

What you say?

@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Feb 12, 2016

Thanks for the issue @zxqfox.

My (perhaps strong) opinion here is that if you need conditionals inside of your test logic, you're probably testing wrong.

To elaborate - why would you need to determine if something is one of two types, or one of two values? If your application logic can return multiple types or values, then your test logic should be calling the functions enough to individually test each branch. As an example:

function doesTwoThings(someVar) {
  if (someVar === 1) {
    return 'hello!';
  } else {
    return [1,2,3];
  }
}

Now we could test that logic with conditionals

expect(doesTwoThings(1)).to.be.either.an('array').or.a('string');

Alternatively - and this is a much better way of doing this - we test both branches of our app logic:

expect(doesTwoThings(1)).to.be.a('string');
expect(doesTwoThings(2)).to.be.an('array');

Question: Do you have a valid use case for this that isn't testing application logic branches?

I'm going to close this for now, but thats mostly for bookkeeping. Please feel free to continue the discussion, and if there is enough of a compelling case made, then we can re-open it.

@keithamus keithamus closed this Feb 12, 2016

@thomasvanlankveld

This comment has been minimized.

Copy link

thomasvanlankveld commented May 9, 2016

Just some thoughts:

Although not exactly the case presented above, I can present an example based on a real-world problem that I ran into:

Feature: See list of posts

Scenario: Post deleted
  Given author "Timothy" wrote a post "A foo and a baz walk into a bar"
  And "Timothy" deleted his post "A foo and a baz walk into a bar"
  When I log in as an admin
  Then I see that "Timothy" did not author the post "A foo and a baz walk into a bar"

Scenario: Post authorship changed
  Given author "Timothy" wrote a post "A foo and a baz walk into a bar"
  And "Timothy" transferred authorship of "A foo and a baz walk into a bar" to "Kyle"
  When I log in as an admin
  Then I see that "Timothy" did not author the post "A foo and a baz walk into a bar"

I can verify authorship be checking an authorId on the post object; post.authorId. In the first case, there will not be a post object, but in the the second case the post object will not have Timothy's id as authorId, so basically either of two predicates should pass.

expect(post).to.satisfy.either([
  post => typeof post === 'undefined' || post === null,
  post => post.authorId !== authorId,
]);

Could be syntactic sugar for:

function didNotAuthor(post) {
  return [
    post => typeof post === 'undefined' || post === null,
    post => post.authorId !== authorId,
  ].some(predicate => predicate(post));
}

expect(post).to.satisfy(didNotAuthor);

This version still requires me to write an ugly check instead of using the nice expect(thing).to.not.exist though... Best would maybe be:

expect(post).to.satisfy.either([
  post => expect(post).to.not.exist,
  post => expect(post.authorId).to.not.equal(authorId),
]);

Not sure if that's stretching it too far.

What I do now is:

const post = await adminView.postByTitle(postTitle) || { authorId: null };
const author = await adminView.authorByName(authorName);
expect(post.authorId).to.not.equal(author.authorId);

It feels a bit like a hack; it is not explicit about the fact that we're really checking for either of two cases, but it works.

Hope this helps!

@lucasfcosta

This comment has been minimized.

Copy link
Member

lucasfcosta commented May 9, 2016

Hi @thomasvanlankveld, thanks for your feedback!

Well, your opinion makes total sense and I understand your wishes to make syntax clearer, but IMO the satisfy version of your code seems easier to understand in terms of testing every possible condition.
As @keithamus said above, I feel like using either could create a wrong mindset for tests.

In this case satisfy is about testing a single thing against two conditions it should match and not two possible conditional branches.

Let me know if you disagree or if you have any further opinions, it's always good to hear feedback from our users 😄

@zxqfox

This comment has been minimized.

Copy link

zxqfox commented May 9, 2016

Hi @lucasfcosta, I'm not strongly for either since it just a syntactic sugar but I don't get why you against it. That's wrong if you will tell consumers how to write their tests.

E.g. there are set of integrational cases (like @thomasvanlankveld showed) which couldn't be determined accurately. And chai lacks this functionality for them. Probably we can make a plugin for either-or?

Unfortunately I don't remember the first case that forced me to file this issue. ;-(

@keithamus Oh. Sorry for no answer. I've missed notification and forget about this issue.

@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented May 9, 2016

tell consumers how to write their tests

That's pretty much how chai works - and I think it is indeed part of chai's responsibility to guide you into writing "correct" tests (or at least, avert you from writing incorrect ones). We also have limited resource and have to carefully curate what comes into core - this means we have a desire to keep core focussed on, well, core stuff. An either assertion breaks away from Chai's core model, which stops us being focussed.

Probably we can make a plugin for either-or?

I think this is exactly the mindset I'd like for us to be after. In an ideal world - I'd like for chai users to create plugins for all the assertions they want, and chai-core periodically vacuums up the ones 90% of users install. Plugins become useful to either A) pick up domain or framework specifics (e.g. chai-react) or B) break out of chai's core model, for better or worse (chai-as-promised, chai-things, and the theoretical either-or you talk about).

@meeber

This comment has been minimized.

Copy link
Contributor

meeber commented May 9, 2016

I would strongly urge you to reconsider how you're structuring your tests rather than write a plugin for adding or functionality.

If you find yourself needing or in your tests, it suggests one of two things:

  1. Your tests are non-deterministic. In other words, each time you run your tests, you potentially get different results based on external conditions or pseudo-randomness. Rather than write tests that attempt to handle varying results by using or, I'd advise refactoring your tests, using test doubles as needed, in order to make your tests deterministic. The goal is for your tests to produce the exact same result each time they're run. And then, as @keithamus exemplified in his first reply, a separate test should be written with different inputs (or different test doubles) in order to validate that the expected result is produced under each set of circumstances. Here's a pretty good resource on this topic: http://martinfowler.com/articles/nonDeterminism.html
  2. You're trying to reuse an assertion for multiple test cases. I understand the allure of DRY, but a certain degree of repeating yourself is necessary when it comes to writing tests. In @thomasvanlankveld's example, I'd recommend writing two completely separate tests, one that sets up the "post deleted" conditions, and then tests specifically that the desired outcome is produced, and then one that sets up the "post authorship changed" conditions, and then tests specifically that the desired outcome is produced. Even though both tests are testing identical outcomes, each test should be completely independent; they shouldn't reuse the same assertion, since that would require the assertion to be able to handle multiple scenarios with or.
@thomasvanlankveld

This comment has been minimized.

Copy link

thomasvanlankveld commented May 10, 2016

Thanks for the suggestions!

A small addendum to my examples above:

When satisfy fails, it does not tell you why. So my example above using the didNotAuthor function, you'll only just hear that the whole thing failed. For two conditions, this is not so bad but if the list of conditions grows larger you're gonna have a bad time.

Reviewing the code where I originally had the problem, I see that @meeber is actually right in his second suggestion, and I can split the assertion into two separate ones. Reworked into the example above, I can change the assertion of the first test:

Feature: See list of posts

Scenario: Post deleted
  Given author "Timothy" wrote a post "A foo and a baz walk into a bar"
  And "Timothy" deleted his post "A foo and a baz walk into a bar"
  When I log in as an admin
  Then I see that there is no post named "A foo and a baz walk into a bar"

Scenario: Post authorship changed
  Given author "Timothy" wrote a post "A foo and a baz walk into a bar"
  And "Timothy" transferred authorship of "A foo and a baz walk into a bar" to "Kyle"
  When I log in as an admin
  Then I see that "Timothy" did not author the post "A foo and a baz walk into a bar"
@giobero

This comment has been minimized.

Copy link

giobero commented Nov 7, 2016

If would be great if satisfy (both or either of them):

  • took first optional parameter failMsg
  • if test function returned String value then that should be considered as failMsg

And use of failMsg would be the reason of failure displayed in report...

Thank you for your great work!

@lucasfcosta

This comment has been minimized.

Copy link
Member

lucasfcosta commented Nov 8, 2016

Hi @giobero, thanks for your feedback!

I'm not sure if I really understand what you mean, but actually this should be addressed in Mocha, not in Chai, since Mocha is the test runner and we're only responsible for the assertions.
If you want to pass a fail message for a single assertion you can do it, but you can't choose what message will be displayed when the whole test fails.

Let me know if I misunderstood what you've said, thanks again for your input! 😄

@chovy

This comment has been minimized.

Copy link

chovy commented Feb 4, 2017

So I have a foo.status property which can either be an integer or a string. is there anyway to write an expect(foo.status) that will pass when its either 200 or 'some status message'?
Something like this:

    expect(item).to.have.property('status')
      .that.is.either.a('string').or.a('number');
@keithamus

This comment has been minimized.

Copy link
Member

keithamus commented Feb 4, 2017

@chovy I would strongly recommend writing tests that assert it is a number when it is a number and a string when it is a string. If you are unable to determine what type foo.status will be for a given input, it may be indicative of problems in your codebase. See my original comment above

@chovy

This comment has been minimized.

Copy link

chovy commented Feb 4, 2017

I changed my tac and now have error in foo.error instead of foo.status. problem solved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment