fix(#2655): Race condition in activity status

[Curiousite]

Changelog Entry

Fixed

• Fixes #2655. "Taking longer than usual to connect" should not show up after reconnect succeeded, by @curiousite and @compulim in PR #2656

Description

Added check to prevent race-condition in activity status.

Specific Changes

Now state will only transfer to "connectingslow" if the previous state is "reconnecting".

---

• Testing Added

[Curiousite]

This is probably more of a workaround than a fix, since it doesn't address the underlying problem. It probably adds stability anyway since a state transition from anything else than "reconnecting" to "connectingslow" doesn't seem to make a lot of sense.

[cwhitten]

Hi @Curiousite, the failing test is below.

FAIL __tests__/offlineUI.js (118.225s)
  ● offline UI › should show "Taking longer than usual to connect" UI when connection is slow

    Expected image to match or be a close match to snapshot but was 0.1918402777777778% different from snapshot (442 differing pixels).
    �[1m�[31mSee diff for details:�[39m�[22m �[31m/home/vsts/work/1/s/__tests__/__image_snapshots__/chrome-docker/__diff_output__/offline-ui-js-offline-ui-should-show-taking-longer-than-usual-to-connect-ui-when-connection-is-slow-1-diff.png�[39m

      73 |     const base64PNG = await driver.takeScreenshot();
      74 | 
    > 75 |     expect(base64PNG).toMatchImageSnapshot(imageSnapshotOptions);
         |                       ^
      76 |   });
      77 | 
      78 |   test('should show "unable to connect" UI when credentials are incorrect', async () => {

      at _callee$ (__tests__/offlineUI.js:75:23)
      at tryCatch (node_modules/regenerator-runtime/runtime.js:45:40)
      at Generator.invoke [as _invoke] (node_modules/regenerator-runtime/runtime.js:271:22)
      at Generator.prototype.(anonymous function) [as next] (node_modules/regenerator-runtime/runtime.js:97:21)
      at asyncGeneratorStep (node_modules/@babel/runtime/helpers/asyncToGenerator.js:3:24)
      at _next (node_modules/@babel/runtime/helpers/asyncToGenerator.js:25:9)

[Curiousite]

Hi @cwhitten, thnx for the reply. How can I run those tests locally? When I try npm test, I am greeted with the error message: "ECONNREFUSED connect ECONNREFUSED 127.0.0.1:4444" for most of the tests. I didn't find additional information on testing in the CONTRIBUTING.md

When I tested the behaviour itself it seems to work. After 15s the warning appears.

[corinagum]

@Curiousite, please take a look at the README.md found in __tests__. If you could also take a look at CONTRIBUTING.md, that would be great.
With your fixes, could you also add an entry to the CHANGELOG.md? Please follow the format of the 'Fix' example at the top, and add it to the Unreleased Fixed section. Thank you!

[msftclas]

All CLA requirements met.

[Curiousite]

Hi @corinagum,
thanks for the fast reply and pointing me in the right direction. The tests should work now and I've included the the line in CHANGELOG.md.

[coveralls]

Coverage increased (+0.02%) to 63.712% when pulling dd1b429 on Curiousite:master into 0fcd7fe on microsoft:master.

[compulim]

Although your fix is a workaround, it is also defensive programming. 😉

ECONNREFUSED localhost:4444 is because it tries to hit your local Docker. We use Chrome inside Docker to run tests for stability. If you follow Corina's suggestion, you should be able to set it up.

Can you add a test to show it's working? We write E2E tests and you can follow __tests__/offlineUI.js. You can consider: forcing Web Chat into error, and dispatch CONNECT_STILL_PENDING, and check that the UI is still showing error when your fix is applied. And when you remove your fix, the test should fail.

[Curiousite]

Hi @compulim,
you're right, thanks. 😄

I tried to implement an E2E-Test, but somehow I can't wrap my head around the test suite. For example, when I setup a test like this (copy-pasted from the reconnection test):

test('should not show "Taking longer than usual to connect" when connection is (re/)established', async () => {
    const { driver } = await setupWebDriver({
      createDirectLine: options => {
        const reconnectingDirectLine = window.WebChat.createDirectLine(options);

        return {
          activity$: reconnectingDirectLine.activity$,
          postActivity: reconnectingDirectLine.postActivity.bind(reconnectingDirectLine),

          connectionStatus$: new Observable(observer => {
            const subscription = reconnectingDirectLine.connectionStatus$.subscribe({
              complete: () => observer.complete(),
              error: err => observer.error(err),
              next: connectionStatus => {
                observer.next(connectionStatus);
                // connectionStatus === ONLINE && observer.next(CONNECTING);
              }
            });

            return () => subscription.unsubscribe();
          })
        };
      },
      pingBotOnLoad: false,
      setup: () =>
        Promise.all([
          window.WebChatTest.loadScript('https://unpkg.com/[email protected]/client/core.min.js'),
          window.WebChatTest.loadScript('https://unpkg.com/[email protected]/lolex.js')
        ]).then(() => {
          window.WebChatTest.clock = lolex.install();
        })
    });

    await driver.executeScript(() => {
      window.WebChatTest.clock.tick(400); // "Connecting" will be gone after 400ms
      window.WebChatTest.clock.tick(14600); // Go to t=15s
    });
    
    await driver.executeScript(() => {
      window.WebChatTest.clock.tick(1); // Shortly after 15s, it will show "Taking longer than usual to connect"
    });
    
    const base64PNG = await driver.takeScreenshot()
    expect(base64PNG).toMatchImageSnapshot(imageSnapshotOptions);
  });

The screenshot shows the "Taking longer than usual to connect" message. But I just pass through the connectionstatus. Shouldn't it then be connected after the initial login? It doesn't seem to make a difference if the commented out line is in there or not. 🤔

[compulim]

I am updating your PR with a fix that target the root cause. Will rebase and merge in soon.

[compulim]

Rebased. Will merge once test succeeded.

[Curiousite]

@compulim Great! Thanks for looking into this and fixing it. 💯 🎉