Chi 1967 productionize lex pre survey chatbot test

[acedeywin]

Description

• Chore: Adding unit tests for lex pre-survey chatbot

Checklist

• Corresponding issue has been opened
• New tests added

Related Issues

Fixes CHI-1967

Verification steps

[gpaoloni]

@acedeywin The issue you are currently having here is related to how you are mocking things. See that in chatbotCallback.test, you are mocking like

getTwilioClient: jest.fn().mockReturnValue({
    chat: {
      services: jest.fn().mockReturnValue({
         ......

while in captureChannelWithBot.test, you are mocking like

getTwilioClient: jest.fn().mockReturnValue(() => ({
    chat: {
      v2: {
         ......

The second means getTwilioClient is a function that returns a function. You either must use mockImplementation if you want the function shape of the mock, or mockReturnValue if you want to just return an object. The first comes handy if you need to return different objects (or throw errors) depending on the test scenarios. Speaking of that, it would be great to have some more tests on this two particular functions and/or testing that the functions we expect to be called are indeed called - is cool that the return state is 200 if everything is ok, but it's also important to check the channel attributes are updated, the child's messages are sent to Lex and vice versa. Happy to talk more about this if in doubt!

[gpaoloni]

@acedeywin Also, can you merge this PR against my branch instead than master? I'd prefer to have the other one merged against master, cause there's a lot of important details in the description.

[acedeywin]

@acedeywin The issue you are currently having here is related to how you are mocking things. See that in chatbotCallback.test, you are mocking like

getTwilioClient: jest.fn().mockReturnValue({
    chat: {
      services: jest.fn().mockReturnValue({
         ......

while in captureChannelWithBot.test, you are mocking like

getTwilioClient: jest.fn().mockReturnValue(() => ({
    chat: {
      v2: {
         ......

The second means getTwilioClient is a function that returns a function. You either must use mockImplementation if you want the function shape of the mock, or mockReturnValue if you want to just return an object. The first comes handy if you need to return different objects (or throw errors) depending on the test scenarios. Speaking of that, it would be great to have some more tests on this two particular functions and/or testing that the functions we expect to be called are indeed called - is cool that the return state is 200 if everything is ok, but it's also important to check the channel attributes are updated, the child's messages are sent to Lex and vice versa. Happy to talk more about this if in doubt!

@gpaoloni - I have tried implementing the mock both as a function using mockImplementation and as an object using mockReturnValue, and I am getting the same result. How do I test the captureChannelWithBot function to see its output? I know this task is one of the many tasks, but how did you go about testing?

[acedeywin]

@acedeywin Also, can you merge this PR against my branch instead than master? I'd prefer to have the other one merged against master, cause there's a lot of important details in the description.

@gpaoloni - Yes, that has been done.

[acedeywin]

@gpaoloni - Finally got to the root cause of why the unit tests for captureChannelWithBot and chatbotCallback were not returning 200 statusCode.

The major issues I encountered were properly mocking handlerPath and lexClient. With further investigation, I implemented the mock for handlerPath, and @stephenhand helped out with that for lexClient. But we were still getting 500 statusCode, and this was caused by the variable blow declaration:
const lexClient = require(handlerPath).addCustomerExternalId as LexClient
Further investigation relieved that addCustomerExternalId does not exist in the lexCilent.private file; hence it will always return undefined. So refactoring to const lexClient = require(handlerPath) as LexClient worked perfectly as we could now access lexClient properties. Do you think I am missing anything as to why you added .addCustomerExternalId?

[stephenhand]

@acedeywin @gpaoloni - this PR will be easiuer to review if both the PR branch and the target branch have Murilo's asset changes merged in :-)

[acedeywin]

@acedeywin @gpaoloni - this PR will be easiuer to review if both the PR branch and the target branch have Murilo's asset changes merged in :-)

@stephenhand - It's done

[stephenhand]

This is always true?

[acedeywin]

@stephenhand - It can be false but for us to to be able to test the codes within the if block, we must check that EventType === 'onMessageSent' && fromServiceUser === From.
The should handle the event and ignore it is to test when EventType === 'onMessageSent' && fromServiceUser === From is false

[stephenhand]

But for this test, you explicitly set it to 'onMessageSent' at the start of the test, so there's no point in checking it here, we know it's set to that already.

The same with the attributes.fromServiceUser, the attributes set up as a response to a mock in this test, so they will always have the expected values, the 'if' condition is only checking the value of your own mock against some test data that also gets assigned in the test, not any production code. You could remove the call to the mock on line 155 and this check and the testwould be still testing the same production behaviour.

Generally 'if' blocks in tests should be considered a smell. If this 'if' condition is ever false, it will result in the rest of the test being bypassed completely and passing because none of the assertions get checked. I assume this wouldn't be desirable, we should actually fail the test if this condition was ever not true, so the if statement would be replaced with some assertions.

But in this case I don't think the checks in the 'if' condition are adding any value to the test at all and it can just be removed

[acedeywin]

@stephenhand - I see what you mean in the comment. I can set From to equal attributes.fromServiceUser, and since EventType is already onMessageSent, the test will pass—the if block is unnecessary. I have gone ahead and made the necessary changes.

[stephenhand]

It is pointless to verify a method that is called directly from the test, this doesn't verify anything about the production code.

In fact it looks like none of the stuff around creating a mock message would maqke any difference, because it's all mocked - apart from the verification on line 195 that looks to test some production behaviour, the other bit just looks to set up some mocked code and then test the mocks?

[acedeywin]

@stephenhand - Yes, Steve. You are absolutely correct. I was using it to test if that actual mock would work as expected. I will go ahead and remove it.

[stephenhand]

LGTM!