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

String should appear n times in another string #971

Closed
lithin opened this issue May 24, 2017 · 8 comments
Closed

String should appear n times in another string #971

lithin opened this issue May 24, 2017 · 8 comments

Comments

@lithin
Copy link

lithin commented May 24, 2017

I have seen myself write this assertion today and couldn't find a better way with chai. Might be worth implementing?

result.match(/BBQ Dip/g).should.have.length(3);

I'm happy to do that if you find it useful.

@meeber
Copy link
Contributor

meeber commented May 27, 2017

@lithin Thanks for the post. What's the syntax that you're thinking about for this?

@lithin
Copy link
Author

lithin commented May 29, 2017

Thanks for getting back to me @meeber :) I was thinking something like:

result.should.contain('BBQ Dip').times(3);

Do you think that's a good way to go about it?

@meeber
Copy link
Contributor

meeber commented May 31, 2017

I'm not yet sure how I feel about this. Some scattered thoughts and questions to ponder:

  • There are a couple of ways to assert the repeated inclusion of a substring without introducing a new .times assertion: the way you posted originally with regular expressions combined with the .length assertion, as well as using only regular expressions via the .match assertion. Given the alternatives, is asserting the repeated inclusion of a string common enough to justify a new assertion?
  • This assertion may suffer from the same problem as .not.have.keys incorrectly failing #919 (comment); does .times(3) mean "exactly three times", or does it mean "at least three times"? Is that meaning consistent with the rest of Chai? Should the issue described in that comment be solved before adding this new assertion?
  • Assertions cannot look ahead in the chain, so it's unavoidable that .contain would first look for the substring in the target a single time before reaching the .times assertion afterward. This is especially problematic when combined with the .not operator, as expect("foofoofoo").to.not.contain("foo").times(10) would throw an assertion error before even reaching the .times part. Instead, the assertion would need to be written expect("foofoofoo").to.contain("foo").but.not.times(10). Is that okay?
  • If .times works with .contain, then it should also work with .string.
  • Should .times work with any other assertions? Does it mislead or create confusion when combined with any other assertions? (I didn't see any during a quick glance through.)

@lithin
Copy link
Author

lithin commented Jun 1, 2017

Hey I totally agree with the points you raised - even when I was suggesting.times, I felt really unsure about this chaining strategy. I still feel however, that having an assertion for this in chai would be helpful, even if there are work-arounds.

Probably something like expect("foofoofoo").to.contain.n("foo", 3) would work better?

I'd say that it needs to be "exactly three times" as opposed to "at least three times". If we're using not, then the assertion would become really confusing. Then expect("foofoofoo").to.contain.n("foo", 3) passes, expect("foofoofoofoo").to.contain.n("foo", 3) fails, and expect("foofoofoofoo").not.to.contain.n("foo", 3) passes. We could also have expect("foofoofoo").to.contain.exactly.n("foo", 3) but that seems to make it unnecessarily complicated.

I think this could work with all assertions that check that "something is there", adding "something is there exactly this many times".

@lucasfcosta
Copy link
Member

lucasfcosta commented Jun 7, 2017

Hi, @lithin, thanks for your issue!

I've thought about this for a bit of time and I'm not sure we should accept it for the following reasons:

  • It's fairly easy to overcome this with Regular Expressions, even though it does not look so elegant
  • It might encourage non-deterministic tests. I can't think of valid use cases for wanting to check if a string contains another string x times instead of just checking if the output string is precisely equal to another value.

@meeber and other contributors have discussed this in other assertions and we've faced problems with similar approaches that encourage "loose" assertions, so I'd like to avoid adding any similar features.

By taking this approach I think we will be trying to "educate" users to avoid this kind of assertion.

You can read more about this at #892 and I've also made a related blog post a while ago.

Recently we have even added warnings in our docs to avoid using not due to the same reasons mentioned before.

However I'd like to offer you my support if you'd like to create a plugin with a few String related assertions, such as this one if it fits your needs.

If any other members disagree, please let me know and thanks again for your issue @lithin 😄

Edit: Just thought a bit more about this and I think it could be useful for long texts, even though we could compare them using equals, it would be a bit problematic. Maybe if we take some more time to think through this and come up with a good syntax it could be a good addition the core (even though I'm not sure about that).

If we decide on adding this, I thought about using the existing string assertion, but like this: string(str[, qtd, msg]).

Please let me know what you think, I'm seriously in doubt.

@keithamus
Copy link
Member

I'll add to this; that if you wanted to - you could always use the chai-string plugin which actually has an entriesCount method which will do what you're after.

@vieiralucas
Copy link
Member

Thank you for the issue

IMHO this looks like plugin land.

As @keithamus said, it looks like chai-string covers this

@keithamus
Copy link
Member

So @lithin hopefully this gives you some good answers and some context about adding it into core.

I think I'll close this issue for now. We have the chai-strings plugin, and we try to make a conscious effort to not copy what plugins have done into core. If you want to continue the discussion, please feel free to do so and we can always re-open if there is strong evidence to add into core 😄.

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

No branches or pull requests

5 participants