fix(#2655): Race condition in activity status#2656
fix(#2655): Race condition in activity status#2656compulim merged 4 commits intomicrosoft:masterfrom Curiousite:master
Conversation
|
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. |
|
Hi @Curiousite, the failing test is below. |
|
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. |
|
@Curiousite, please take a look at the README.md found in |
|
Hi @corinagum, |
|
Although your fix is a workaround, it is also defensive programming. 😉
Can you add a test to show it's working? We write E2E tests and you can follow |
|
Hi @compulim, 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. 🤔 |
|
I am updating your PR with a fix that target the root cause. Will rebase and merge in soon. |
|
Rebased. Will merge once test succeeded. |
|
@compulim Great! Thanks for looking into this and fixing it. 💯 🎉 |
Changelog Entry
Fixed
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".