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

Show custom error messages #52

Merged
merged 5 commits into from
Jan 19, 2018

Conversation

santiagodoldan
Copy link
Contributor

No description provided.

microsoftly and others added 2 commits December 6, 2017 18:34
* Add message filter and in-line options update (microsoftly#28)

* added ability to filter messages

* added in line options modifier

* add builtin message filters

* add builtin message filters to options/config (microsoftly#30)

* v3.1.0 (microsoftly#32)

* add builtin message filters

* bump version for v3.1.0

* updated docs for publish

* added ignoreInternalSaveMessage option

* Add internal save ignore option (microsoftly#35)

* v3.2.0

* fixed a redundant definition that came from a recursive merge strategy

* fix spelling error with ignoreInternalSaveMessage (microsoftly#38)

* 3.2.1

* remove typescript notation from docs

* forcibly remove docs folder when building typeDocs

* 3.2.2

* add adaptive cards (and attachment) support (microsoftly#44)

* add adaptiveCardProvider

* add adaptivecards package

* add adaptive cards implementation

* add adaptive cards tests

* update docs

* include ts-node require

* update node version

* update mocha version

* add yarn to circle config

* v3.3.0
@santiagodoldan
Copy link
Contributor Author

This should fix this issue #51

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 95.38% when pulling b8bbb01 on suttna:show-custom-error-messages into 14aa478 on microsoftly:master.

@microsoftly microsoftly changed the base branch from master to dev January 19, 2018 00:21
@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.372% when pulling 49b8732 on suttna:show-custom-error-messages into 04f6abe on microsoftly:dev.

@microsoftly
Copy link
Owner

Looks good to me.

Please add a test to BotTester.spec.ts with the title "Can show custom error messages" and add the test to the README.md documentation and I'll be ready to merge and release an updated package.

sidenote: I've changed the base to from master to dev. Once dev is ready for publishing, the docs are recompiled, version number bumped, package is published, then dev is merged into master.

@santiagodoldan
Copy link
Contributor Author

santiagodoldan commented Jan 19, 2018

@microsoftly should I add a tests for each matcher? https://github.com/microsoftly/BotTester/blob/master/src/assertionLibraries/ChaiExpectation.ts#L39-L53 looks like the deep include doesn't use the given message

@microsoftly
Copy link
Owner

@santiagodoldan yes, please explicitly test each method.

I think having deep include also have the message could be useful. It shouldn't be too much work to extend it as well.

Only one of the new tests needs to be added to the documentation. The rest can be put in the cases not for docs

@santiagodoldan
Copy link
Contributor Author

@microsoftly

First of all, there's already a test file https://github.com/microsoftly/BotTester/blob/master/test/BotTesterFailure.spec.ts that test the error messages, the issue I see there is that chai includes both messages in the Error object (the format is "new-message:old-message") and this expectation https://github.com/microsoftly/BotTester/blob/master/test/BotTesterFailure.spec.ts is not comparing if the string is equal, it checks if the error message contains that string, which is true because is the default message that chai uses.

Regarding the test file, I was able to test the toInclude just passing the new custom message, I also was able to test the notToBeEmpty passing an empty array to the sendMessageToBot and also the toBeTrue passing a regexp.

The only one I can't test is the toEqual, I think the line that uses that expectation can't be reached, here you have the code https://github.com/microsoftly/BotTester/blob/master/src/ExpectedMessage.ts#L71-L72 and https://github.com/microsoftly/BotTester/blob/master/src/ExpectedMessage.ts#L71-L72, that default block isn't executed IMO.

@santiagodoldan
Copy link
Contributor Author

@microsoftly another thing, would you like to move this expectation https://github.com/microsoftly/BotTester/blob/master/src/ExpectedMessage.ts#L50 out of the constructor? because that fails without executing runTest()

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 95.254% when pulling 932bdad on suttna:show-custom-error-messages into 04f6abe on microsoftly:dev.

@santiagodoldan
Copy link
Contributor Author

@microsoftly regarding what chai does with the message https://github.com/chaijs/chai/blob/master/lib/chai/utils/getMessage.js#L49, do you want to remove the old message or is that ok? let me know what would you like to do, so I can finish the PR, thanks :)

@microsoftly
Copy link
Owner

Removing the expect out of the constructor is fine.

I'm not sure which old message you're referring to, but I'll let you pick your best judgement. I'm fine W/ the PR as is.

@microsoftly
Copy link
Owner

as for https://github.com/microsoftly/BotTester/blob/master/src/ExpectedMessage.ts#L71-L72, it's been a while, but I'm pretty sure that's for ignoring internal save messages that are used to get the session loader to work properly

@coveralls
Copy link

Coverage Status

Coverage increased (+1.3%) to 95.254% when pulling b0e06c6 on suttna:show-custom-error-messages into f34cfd5 on microsoftly:dev.

@santiagodoldan
Copy link
Contributor Author

@microsoftly

About the message, here you have an example of how it looks

Bot should have responded with 'NOPE', but was 'how are you doing?: expected [ 'NOPE' ] to include 'how are you doing?'

If that's ok, let's merge the PR then :)

@microsoftly
Copy link
Owner

great

@microsoftly microsoftly self-requested a review January 19, 2018 22:29
@microsoftly microsoftly merged commit 3a5c2d8 into microsoftly:dev Jan 19, 2018
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.

None yet

3 participants