Added Nock integration for mocking QnA Tests #367
Conversation
JuanAr
force-pushed
the
southworks/qna/test-mocks
branch
from
5fcb47f to
dd5e24e
Compare
JuanAr
force-pushed
the
southworks/qna/test-mocks
branch
from
dd5e24e to
8aace06
Compare
Pull Request Test Coverage Report for Build 1118
💛 - Coveralls |
| const endpointKey = process.env.QNAENDPOINTKEY; | ||
| const hostname = process.env.QNAHOSTNAME; | ||
| const hostname = process.env.QNAHOSTNAME || 'botbuilder-test-app'; | ||
| const mockQnA = true; |
There was a problem hiding this comment.
Shouldn't this be dependent on earlier variables? Something that looks more like this:
const mockQna = knowledgeBaseId && endpointKey && hostname !== 'botbuilder-test-app';There was a problem hiding this comment.
We'll include this logic. Moreover, we'll include an override flag to force using the mocks.
| return; | ||
| console.warn('WARNING: QnAMaker test suite QNAENDPOINTKEY environment variable is not defined'); | ||
| } | ||
| if (!hostname) { |
There was a problem hiding this comment.
This check is no longer necessary because hostname is always defined via L10.
There was a problem hiding this comment.
Ok, we'll remove this.
| assert(res); | ||
| assert(res.length == 1); | ||
| assert(res[0].answer.startsWith("BaseCamp: You can use a damp rag to clean around the Power Pack")); | ||
| assert.strictEqual(typeof res !== 'undefined', true, 'The response was returned as \'undefined\'.'); |
There was a problem hiding this comment.
This seems needlessly complex... Since I'm assuming the QnAMaker and our API never returns an undefined or null we should just verify that a response exists (look for a truthy value). So instead we should use:
assert(res, `A valid response was not returned.`);Otherwise if we should check for undefined we can use:
assert.strictNotEqual(res, undefined, `The response was returned as 'undefined'.`);There was a problem hiding this comment.
We'll use the second option, to keep consistency between all the asserts.
| return qna.generateAnswer(`how do I clean the stove?`) | ||
| .then(res => { | ||
| assert.strictEqual(typeof res !== 'undefined', true, 'The response was returned as \'undefined\'.'); | ||
| assert.strictEqual(res.length, 1, 'Should have receive just one answer on the first call.'); |
There was a problem hiding this comment.
We'll fix this typo.
- Modified the logic for `mockQnA` flag. - Added an override flag to force the use of mocks. - Removed the warning for missing `hostname` environment variable. - Replaced `assert.strictEqual` with `assert.notStrictEqual` where necessary. - Fixed several typos. - Replaced escaped backslashes with ticks.
|
@stevengum we applied your feedback. Thanks! |
|
@JuanAr, can you look into why the build is failing, and also include an explicit console log that indicates if the mocks (nocks?) are running. |
- additionally fixed minor tracing bug
Tests in
qnaMaker.test.jswere adapted to use Nock and retrieve responses stored in JSON files.assert.strictEqual, and an error message was added for each assert.hostnameenvironment variable is not defined, a default one is used by NockmockQnAwas added to toggle between using the service or the mocked responses