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

Fix bug with greeting [BOTKIT] #59

Merged
merged 11 commits into from
Dec 3, 2019
Merged

Conversation

nikitasakau
Copy link
Member

@nikitasakau nikitasakau commented Nov 25, 2019

Fix #53 #31
image

  • Updated botbuilder-adapter-slack, botkit, botkit-mock in order to use middlewares in specs (Only the last version botkit-mock supports them)
  • Changed some styles in specs as in examples of botkit-mock
  • Fixed bug with greeting as we fixed it in master
  • Created spec that cover this bug

@nikitasakau nikitasakau self-assigned this Nov 25, 2019
@nikitasakau nikitasakau changed the title Fix bug with greeting Fix bug with greeting [BOTKIT] Nov 25, 2019
@nikitasakau nikitasakau added BOTKIT bug infra Changes in infrastructure: performance, refactoring, etc and removed BOTKIT bug labels Nov 25, 2019
Copy link
Contributor

@AleksSenkou AleksSenkou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to see tests rebuilt 👍

specs/greeting_spec.js Outdated Show resolved Hide resolved
messages: [{
text: '```hi```', isAssertion: true,
text: 'they', isAssertion: true,
}],
}]).then(message => assert.deepEqual(message, {}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thoughts about moving this method to a helper method for tests? It's duplicated lot of times

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AleksSenkou Do you mean message => assert.deepEqual(message, {}) only or the whole usersInput?

Copy link
Contributor

@AleksSenkou AleksSenkou Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nikitasakov I'm thinking about:

await userInputs('they').then(message => assert.deepEqual(message, {}))

thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you use await on promise?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or:

await userInputs('they').then(noAnswerFromBotExpected)

Copy link
Member Author

@nikitasakau nikitasakau Dec 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lazycoder9 I did as in docs - https://github.com/gratifyguy/botkit-mock. Or it is not the same?

@npupko
Copy link
Member

npupko commented Dec 2, 2019

@nikitasakov I'm not sure it related to, but should warnings be removed in this MR?
I.e.

[WARN]  WebClient:1 http request failed

Or does it not affect anything? Everyone usually has a different attitude towards the warnings.

@npupko
Copy link
Member

npupko commented Dec 2, 2019

I need this branch to be merged to finish write tests for order water feature, dudes.
Will cherry-pick it locally for now

@nikitasakau
Copy link
Member Author

@npupko Do you mean that we can merge it for now, but create separate issue for refactoring?

@npupko
Copy link
Member

npupko commented Dec 3, 2019

@nikitasakov Yes, it will be awesome. How much time do you need to complete this issue?

@nikitasakau nikitasakau merged commit 8c22579 into move-to-botkit Dec 3, 2019
@nikitasakau nikitasakau deleted the botkit-fix-greeting branch December 3, 2019 10:52
@AleksSenkou
Copy link
Contributor

@nikitasakov what about this comment? #59 (comment)

@nikitasakau
Copy link
Member Author

@AleksSenkou We decided to refactor it in separate issue. Going to continue discussion there. #61

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug infra Changes in infrastructure: performance, refactoring, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants