Refactor shouldDisconnect logic and add more disconnection unit tests.

src/lib/cloud-manager-hoc.jsx

@@ -34,7 +34,7 @@ const cloudManagerHOC = function (WrappedComponent) {
                 this.connectToCloud();
             }
 
-            if (this.shouldDisconnect(this.props) && !this.shouldDisconnect(prevProps)) {
+            if (this.shouldDisconnect(this.props, prevProps)) {
                 this.disconnectFromCloud();
             }
         }
@@ -49,12 +49,12 @@ const cloudManagerHOC = function (WrappedComponent) {
         shouldConnect (props) {
             return !this.isConnected() && this.canUseCloud(props) && props.isShowingWithId;
         }
-        shouldDisconnect (props) {
+        shouldDisconnect (props, prevProps) {
             return this.isConnected() &&
                 ( // Can no longer use cloud or cloud provider info is now stale
                     !this.canUseCloud(this.props) ||
-                    (this.cloudProvider.projectId !== props.projectId) ||
-                    (this.cloudProvider.username !== props.username)
+                    (props.projectId !== prevProps.projectId) ||
+                    (props.username !== prevProps.username)
                 );
             // TODO need to add provisions for viewing someone
             // else's project in editor mode

test/unit/util/cloud-manager-hoc.test.jsx

@@ -6,7 +6,13 @@ import {mount} from 'enzyme';
 import VM from 'scratch-vm';
 import {LoadingState} from '../../../src/reducers/project-state';
 import CloudProvider from '../../../src/lib/cloud-provider';
-jest.mock('../../../src/lib/cloud-provider');
+const mockCloudProviderInstance = {
+    connection: true,
+    requestCloseConnection: jest.fn()
+};
+jest.mock('../../../src/lib/cloud-provider', () =>
+    jest.fn().mockImplementation(() => mockCloudProviderInstance)
+);
 
 import cloudManagerHOC from '../../../src/lib/cloud-manager-hoc.jsx';
 
@@ -36,6 +42,7 @@ describe('CloudManagerHOC', () => {
         vm = new VM();
         vm.setCloudProvider = jest.fn();
         CloudProvider.mockClear();
+        mockCloudProviderInstance.requestCloseConnection.mockClear();
     });
     test('when it mounts, the cloud provider is set on the vm', () => {
         const Component = () => (<div />);
@@ -50,8 +57,7 @@ describe('CloudManagerHOC', () => {
         );
         expect(vm.setCloudProvider.mock.calls.length).toBe(1);
         expect(CloudProvider).toHaveBeenCalledTimes(1);
-        const cloudProviderInstance = CloudProvider.mock.instances[0];
-        expect(vm.setCloudProvider).toHaveBeenCalledWith(cloudProviderInstance);
+        expect(vm.setCloudProvider).toHaveBeenCalledWith(mockCloudProviderInstance);
     });
 
     test('when cloudHost is missing, the cloud provider is not set on the vm', () => {
@@ -117,8 +123,7 @@ describe('CloudManagerHOC', () => {
         });
         expect(vm.setCloudProvider.mock.calls.length).toBe(1);
         expect(CloudProvider).toHaveBeenCalledTimes(1);
-        const cloudProviderInstance = CloudProvider.mock.instances[0];
-        expect(vm.setCloudProvider).toHaveBeenCalledWith(cloudProviderInstance);
+        expect(vm.setCloudProvider).toHaveBeenCalledWith(mockCloudProviderInstance);
     });
 
     test('projectId change should not trigger cloudProvider connection unless isShowingWithId becomes true', () => {
@@ -143,8 +148,7 @@ describe('CloudManagerHOC', () => {
         });
         expect(vm.setCloudProvider.mock.calls.length).toBe(1);
         expect(CloudProvider).toHaveBeenCalledTimes(1);
-        const cloudProviderInstance = CloudProvider.mock.instances[0];
-        expect(vm.setCloudProvider).toHaveBeenCalledWith(cloudProviderInstance);
+        expect(vm.setCloudProvider).toHaveBeenCalledWith(mockCloudProviderInstance);
     });
 
     test('when it unmounts, the cloud provider is set on the vm', () => {
@@ -159,9 +163,8 @@ describe('CloudManagerHOC', () => {
             />
         );
 
-        expect(CloudProvider).toHaveBeenCalledTimes(1);
-        const cloudProviderInstance = CloudProvider.mock.instances[0];
-        const requestCloseConnection = cloudProviderInstance.requestCloseConnection;
+        expect(CloudProvider).toHaveBeenCalled();
+        const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;
 
         mounted.unmount();
 
@@ -171,4 +174,54 @@ describe('CloudManagerHOC', () => {
         expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
         expect(requestCloseConnection).toHaveBeenCalledTimes(1);
     });
+
+    test('projectId changing should trigger cloudProvider disconnection', () => {
+        const Component = () => <div />;
+        const WrappedComponent = cloudManagerHOC(Component);
+        const mounted = mount(
+            <WrappedComponent
+                cloudHost="nonEmpty"
+                store={store}
+                username="user"
+                vm={vm}
+            />
+        );
+
+        expect(CloudProvider).toHaveBeenCalled();
+        const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;
+
+        mounted.setProps({
+            projectId: 'a different id'
+        });
+
+        expect(vm.setCloudProvider.mock.calls.length).toBe(2);
+        expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
+        expect(requestCloseConnection).toHaveBeenCalledTimes(1);
+
+    });
+
+    test('username changing should trigger cloudProvider disconnection', () => {
+        const Component = () => <div />;
+        const WrappedComponent = cloudManagerHOC(Component);
+        const mounted = mount(
+            <WrappedComponent
+                cloudHost="nonEmpty"
+                store={store}
+                username="user"
+                vm={vm}
+            />
+        );
+
+        expect(CloudProvider).toHaveBeenCalled();
+        const requestCloseConnection = mockCloudProviderInstance.requestCloseConnection;
+
+        mounted.setProps({
+            username: 'a different user'
+        });
+
+        expect(vm.setCloudProvider.mock.calls.length).toBe(2);
+        expect(vm.setCloudProvider).toHaveBeenCalledWith(null);
+        expect(requestCloseConnection).toHaveBeenCalledTimes(1);
+
+    });
 });