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

Warn user when includesText assertion should expect collapsable whitespace #198

Merged
merged 10 commits into from
Dec 7, 2018

Conversation

happycollision
Copy link
Contributor

I added three tests. The first two may not be desirable specifications, since passing them might cause a major version bump again. (They test that text containing expected line breaks, etc, would pass.)

The third test is an example of a possible scenario to mitigate confusion described in #197. That would be my suggestion if passing the first two is too much of a burden. Or something like that.

Three tests are added. The first two may not be desirable specifications,
since passing them might cause a major version bump again.

The third test is an example of a possible scenario to mitigage confusion.
That would be my suggestion if passing the first two is too much of a
burden. Or something like that.
@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 16, 2018

I'm not sure what the issue is here. hasText() and includesText() should behave similarly in general and also with regard to whitespace collapsing. I agree though, that it might make sense to print a warning somewhere if the input string contains whitespace that would be collapsed 🤔

If your issue is that you would like to do a has/includesText() assertion without the automatic collapsing then maybe it would make sense to introduce a collapseWhitespace: false option on those assertions.

@happycollision
Copy link
Contributor Author

happycollision commented Nov 16, 2018 via email

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 16, 2018

Or should it throw and error with a message in there, because bad input was supplied as the expected test?

I think we should probably just use console.warn() 🤔

@happycollision
Copy link
Contributor Author

Great. I think the assertions you have will work just fine. Just that helpful messaging will be sufficient.

I'll do some further tests with changes in an Ember environment, then adjust this PR.

@happycollision happycollision changed the title WIP: Feedback desired: Address issue #197 Warn user when includesText assertion should expect collapsable whitespace Nov 18, 2018
@happycollision
Copy link
Contributor Author

After checking on this inside Ember's testing env, I still think it is possibly more useful to append the warning to the message that is pushed into the results of the assertion. I know that I do not open the inspector when I am testing unless I am looking for something that I am already expecting to see there. Things like console.logs that I have in my code.

To illustrate, here are two screenshots. The first shows adding a warning in the console only.
screen shot 2018-11-18 at 3 37 53 pm

You can see the message is there, but the context for it is difficult to discern. And if you have multiple test failures, you may not know which one it is referring to.

This screenshot shows when we append the warning to the message.
screen shot 2018-11-18 at 3 51 37 pm

Here, we don't need to open the console to see our message associated with the proper test.

This is achieved through adding this code to the assertion just before pushing the message:

    if (!result && text !== collapseWhitespace(text)) {
      message = message + '\n\nYour expected text contains spacing that is not preserved in this assertion. Try the `.hasText()` assertion passing in your expected text as a RegEx pattern.';
    }

So this code won't even run if the result is true.

I am happy to simply change this to console.warn as you suggested, though, if you still prefer that. Let me know if the messaging is what you prefer.

@happycollision
Copy link
Contributor Author

I have also updated the API docs to contain a warning to the same effect. Just lets users know that whitespace is collapsed in this assertion.

@Turbo87
Copy link
Collaborator

Turbo87 commented Nov 21, 2018

I have also updated the API docs to contain a warning to the same effect

I guess the same warning should be added to the hasText() assertion then

I am happy to simply change this to console.warn as you suggested, though, if you still prefer that.

yeah, I think that would be better

@happycollision
Copy link
Contributor Author

happycollision commented Dec 7, 2018

Should be all set, @Turbo87.

FYI, the warning message reads as follows:

The .includesText(), .containsText(), and .hasTextContaining() assertions collapse whitespace. The text you are checking for contains whitespace that may have made your test fail incorrectly. Try the .hasText() assertion passing in your expected text as a RegExp pattern. Your text:

(Then it reproduces the user's text on the next line.)

@Turbo87 Turbo87 merged commit 4dde1f5 into mainmatter:master Dec 7, 2018
@Turbo87
Copy link
Collaborator

Turbo87 commented Dec 7, 2018

thanks :)

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

Successfully merging this pull request may close these issues.

2 participants