Addressed PR comments

diningPhilosopher64 · prabhakk-mw · commit 3ef0b17c86eb · 2025-12-08T15:11:40.000Z
diff --git a/src/jupyter_matlab_kernel/__main__.py b/src/jupyter_matlab_kernel/__main__.py
@@ -5,7 +5,7 @@
     from ipykernel.kernelapp import IPKernelApp
 
     from jupyter_matlab_kernel import mwi_logger
-    from jupyter_matlab_kernel.kernels.kernel_factory import KernelFactory
+    from jupyter_matlab_kernel.kernel_factory import KernelFactory
 
     logger = mwi_logger.get(init=True)
 
diff --git a/src/jupyter_matlab_kernel/base_kernel.py b/src/jupyter_matlab_kernel/base_kernel.py
@@ -27,7 +27,7 @@
 )
 from jupyter_matlab_kernel.mwi_exceptions import MATLABConnectionError
 
-from jupyter_matlab_kernel.kernels.comms import LabExtensionCommunication
+from jupyter_matlab_kernel.comms import LabExtensionCommunication
 
 
 _MATLAB_STARTUP_TIMEOUT = mwi_settings.get_process_startup_timeout()
diff --git a/src/jupyter_matlab_kernel/comms/__init__.py b/src/jupyter_matlab_kernel/comms/__init__.py
diff --git a/src/jupyter_matlab_kernel/comms/labextension/__init__.py b/src/jupyter_matlab_kernel/comms/labextension/__init__.py
diff --git a/src/jupyter_matlab_kernel/comms/labextension/labextension.py b/src/jupyter_matlab_kernel/comms/labextension/labextension.py
diff --git a/src/jupyter_matlab_kernel/jsp_kernel.py b/src/jupyter_matlab_kernel/jsp_kernel.py
@@ -13,7 +13,7 @@
 import aiohttp.client_exceptions
 import requests
 
-from jupyter_matlab_kernel.kernels import base_kernel as base
+from jupyter_matlab_kernel import base_kernel as base
 from jupyter_matlab_kernel import mwi_logger, test_utils
 from jupyter_matlab_kernel.mwi_comm_helpers import MWICommHelper
 from jupyter_matlab_kernel.mwi_exceptions import MATLABConnectionError
diff --git a/src/jupyter_matlab_kernel/kernel_factory.py b/src/jupyter_matlab_kernel/kernel_factory.py
@@ -3,8 +3,8 @@
 import os
 from typing import Union
 
-from jupyter_matlab_kernel.kernels import MATLABKernelUsingJSP
-from jupyter_matlab_kernel.kernels import MATLABKernelUsingMPM
+from jupyter_matlab_kernel.jsp_kernel import MATLABKernelUsingJSP
+from jupyter_matlab_kernel.mpm_kernel import MATLABKernelUsingMPM
 
 
 class KernelFactory:
diff --git a/src/jupyter_matlab_kernel/kernels/__init__.py b/src/jupyter_matlab_kernel/kernels/__init__.py
diff --git a/src/jupyter_matlab_kernel/mpm_kernel.py b/src/jupyter_matlab_kernel/mpm_kernel.py
@@ -9,7 +9,7 @@
 import matlab_proxy_manager.lib.api as mpm_lib
 from requests.exceptions import HTTPError
 
-from jupyter_matlab_kernel.kernels import base_kernel as base
+from jupyter_matlab_kernel import base_kernel as base
 from jupyter_matlab_kernel.mwi_comm_helpers import MWICommHelper
 from jupyter_matlab_kernel.mwi_exceptions import MATLABConnectionError
 
diff --git a/src/jupyter_matlab_labextension/jest.config.js b/src/jupyter_matlab_labextension/jest.config.js
@@ -2,7 +2,7 @@
 module.exports = {
   preset: "ts-jest",
   testEnvironment: "node",
-  testMatch: ["**/tests/**/*.ts?(x)", "**/?(*.)+(spec|test).ts?(x)"],
+  testMatch: ["src/tests/**/*.ts?(x)", "**/?(*.)+(spec|test).ts?(x)"],
   testPathIgnorePatterns: ["/node_modules/", "/src/tests/jest-setup.ts"],
   moduleFileExtensions: ["ts", "tsx", "js", "jsx", "json", "node"],
   setupFilesAfterEnv: ["<rootDir>/src/tests/jest-setup.ts"],
diff --git a/src/jupyter_matlab_labextension/src/tests/matlabCommunication.test.ts b/src/jupyter_matlab_labextension/src/tests/matlabCommunication.test.ts
@@ -53,9 +53,9 @@ describe('MatlabCommunicationExtension', () => {
                                 done: {
                                     then: jest.fn().mockImplementation((cb) => cb()),
                                     catch: jest.fn().mockImplementation((cb) => cb())
-                                    // catch: jest.fn(),
                                 }
-                            })
+                            }),
+                            close: jest.fn()
                         }))
                     }
                 }
@@ -107,25 +107,20 @@ describe('MatlabCommunicationExtension', () => {
         const disposable = extension.createNew(panel, context);
 
         await panel.sessionContext.ready;
-
-        // Wait for the async operations in createNew to complete
         await new Promise((resolve) => setTimeout(resolve, 10));
 
-        // Verify that a comm was created and stored
-        // (We need to check this before disposal since getComm throws if not found)
+        // Verify a comm was created
         const { kernel } = panel.sessionContext.session!;
         expect(kernel?.createComm).toHaveBeenCalled();
 
-        // Simulate panel disposal by calling the disposal callback that was connected
-        // The panel.disposed.connect mock should have been called with a callback
-        expect(panel.disposed.connect).toHaveBeenCalled();
-        const disposalCallback = (panel.disposed.connect as jest.Mock).mock
-            .calls[0][0];
-        disposalCallback(); // Trigger the disposal
+        // Comm should exist before disposal
+        expect(() => extension.getComm(panel.id)).not.toThrow();
+
+        // Simulate disposal via DisposableDelegate
+        disposable.dispose();
 
-        // After disposal, the comm should be cleaned up
+        // Comm should now be cleaned up
         expect(() => extension.getComm(panel.id)).toThrow();
-        expect(disposable.dispose).toBeDefined();
     });
 
     it('should throw an error if getComm is called with an invalid notebook ID', async () => {
diff --git a/tests/unit/jupyter_matlab_kernel/comms/test_labextension.py b/tests/unit/jupyter_matlab_kernel/comms/test_labextension.py
@@ -1,7 +1,7 @@
 # Copyright 2025 The MathWorks, Inc.
 
 import pytest
-from jupyter_matlab_kernel.kernels.comms.labextension import (
+from jupyter_matlab_kernel.comms.labextension import (
     LabExtensionCommunication,
 )
 
@@ -58,7 +58,7 @@ def test_comm_open_creates_comm(
     # Arrange
     # Mock the Comm class
     mock_comm_class = mocker.patch(
-        "jupyter_matlab_kernel.kernels.comms.labextension.labextension.Comm",
+        "jupyter_matlab_kernel.comms.labextension.labextension.Comm",
         return_value=mock_comm,
     )
 
diff --git a/tests/unit/jupyter_matlab_kernel/test_kernel.py b/tests/unit/jupyter_matlab_kernel/test_kernel.py
@@ -6,7 +6,7 @@
 from jupyter_server import serverapp
 from mocks.mock_jupyter_server import MockJupyterServerFixture
 
-from jupyter_matlab_kernel.kernels.jsp_kernel import (
+from jupyter_matlab_kernel.jsp_kernel import (
     MATLABKernelUsingJSP,
     start_matlab_proxy,
 )
diff --git a/tests/unit/jupyter_matlab_kernel/test_kernel_factory.py b/tests/unit/jupyter_matlab_kernel/test_kernel_factory.py
@@ -2,9 +2,9 @@
 
 import pytest
 
-from jupyter_matlab_kernel.kernels import MATLABKernelUsingJSP
-from jupyter_matlab_kernel.kernels import KernelFactory
-from jupyter_matlab_kernel.kernels import MATLABKernelUsingMPM
+from jupyter_matlab_kernel.jsp_kernel import MATLABKernelUsingJSP
+from jupyter_matlab_kernel.kernel_factory import KernelFactory
+from jupyter_matlab_kernel.mpm_kernel import MATLABKernelUsingMPM
 
 
 @pytest.mark.parametrize(
diff --git a/tests/unit/jupyter_matlab_kernel/test_mpm_kernel.py b/tests/unit/jupyter_matlab_kernel/test_mpm_kernel.py
@@ -4,7 +4,7 @@
 
 import pytest
 
-from jupyter_matlab_kernel.kernels import MATLABKernelUsingMPM
+from jupyter_matlab_kernel.mpm_kernel import MATLABKernelUsingMPM
 from jupyter_matlab_kernel.mwi_exceptions import MATLABConnectionError
 
 
@@ -15,11 +15,11 @@ def mpm_kernel_instance(mocker) -> MATLABKernelUsingMPM:
 
     # Use pytest-mock's mocker fixture to patch the function and log attribute
     mocker.patch(
-        "jupyter_matlab_kernel.kernels.base_kernel.BaseMATLABKernel._extract_kernel_id_from_sys_args",
+        "jupyter_matlab_kernel.base_kernel.BaseMATLABKernel._extract_kernel_id_from_sys_args",
         return_value=uuid.uuid4().hex,
     )
     mocker.patch(
-        "jupyter_matlab_kernel.kernels.base_kernel.BaseMATLABKernel.log",
+        "jupyter_matlab_kernel.base_kernel.BaseMATLABKernel.log",
         new=mock_logger,
     )
 
@@ -74,7 +74,7 @@ async def test_initialize_mwi_comm_helper(mocker, mpm_kernel_instance):
 
     # Mock MWICommHelper
     mock_mwi_comm_helper = mocker.patch(
-        "jupyter_matlab_kernel.kernels.mpm_kernel.MWICommHelper", autospec=True
+        "jupyter_matlab_kernel.mpm_kernel.MWICommHelper", autospec=True
     )
     mock_mwi_comm_helper_instance = mock_mwi_comm_helper.return_value
     mock_mwi_comm_helper_instance.connect = mocker.AsyncMock()
diff --git a/tests/unit/jupyter_matlab_kernel/test_mwi_comm_helpers.py b/tests/unit/jupyter_matlab_kernel/test_mwi_comm_helpers.py
@@ -236,14 +236,18 @@ async def mock_post(*args, **kwargs):
 async def test_send_eval_request_to_matlab_success(monkeypatch, matlab_proxy_fixture):
     """Test that send_eval_request_to_matlab returns eval response correctly."""
 
+    # Arrange
     async def mock_post(*args, **kwargs):
         return MockEvalResponse(is_error=False, response_str="", message_faults=[])
 
     monkeypatch.setattr(aiohttp.ClientSession, "post", mock_post)
 
     mcode = "x = 1 + 1"
+
+    # Act
     result = await matlab_proxy_fixture.send_eval_request_to_matlab(mcode)
 
+    # Assert
     # Verify the eval response is returned as-is
     expected_response = {"isError": False, "responseStr": "", "messageFaults": []}
     assert result == expected_response
@@ -254,6 +258,7 @@ async def test_send_eval_request_to_matlab_with_error(
 ):
     """Test that send_eval_request_to_matlab returns error response correctly."""
 
+    # Arrange
     async def mock_post(*args, **kwargs):
         return MockEvalResponse(
             is_error=True,
@@ -264,8 +269,11 @@ async def mock_post(*args, **kwargs):
     monkeypatch.setattr(aiohttp.ClientSession, "post", mock_post)
 
     mcode = "invalid_syntax"
+
+    # Act
     result = await matlab_proxy_fixture.send_eval_request_to_matlab(mcode)
 
+    # Assert
     # Verify the error response is returned as-is
     expected_response = {
         "isError": True,
@@ -279,7 +287,7 @@ async def test_send_eval_request_to_matlab_bad_request(
     monkeypatch, matlab_proxy_fixture
 ):
     """Test that send_eval_request_to_matlab raises exception for bad HTTP request."""
-
+    # Arrange
     mock_exception_message = "Mock exception thrown due to bad request status."
 
     async def mock_post(*args, **kwargs):
@@ -288,8 +296,12 @@ async def mock_post(*args, **kwargs):
     monkeypatch.setattr(aiohttp.ClientSession, "post", mock_post)
 
     mcode = "x = 1 + 1"
+
+    # Act
     with pytest.raises(aiohttp.client_exceptions.ClientError) as exceptionInfo:
         await matlab_proxy_fixture.send_eval_request_to_matlab(mcode)
+
+    # Assert
     assert mock_exception_message in str(exceptionInfo.value)
 
 
@@ -298,6 +310,7 @@ async def test_send_eval_request_to_matlab_missing_eval_response(
 ):
     """Test that send_eval_request_to_matlab raises MATLABConnectionError for missing EvalResponse."""
 
+    # Arrange
     async def mock_post(*args, **kwargs):
         return MockEvalResponseMissingData()
 
@@ -311,7 +324,7 @@ async def mock_post(*args, **kwargs):
 # Testing _read_eval_response_from_file
 async def test_read_eval_response_from_file_success_with_file(matlab_proxy_fixture):
     """Test _read_eval_response_from_file with successful response and file."""
-
+    # Arrange
     # Create a temporary file with test data
     test_data = [
         {"type": "stream", "content": {"name": "stdout", "text": "Hello World"}}
@@ -327,8 +340,10 @@ async def test_read_eval_response_from_file_success_with_file(matlab_proxy_fixtu
             "messageFaults": [],
         }
 
+        # Act
         result = await matlab_proxy_fixture._read_eval_response_from_file(eval_response)
 
+        # Assert
         # Verify the result
         assert result == test_data
 
@@ -343,15 +358,17 @@ async def test_read_eval_response_from_file_success_with_file(matlab_proxy_fixtu
 
 async def test_read_eval_response_from_file_success_without_file(matlab_proxy_fixture):
     """Test _read_eval_response_from_file with successful response but no file."""
-
+    # Arrange
     eval_response = {
         "isError": False,
         "responseStr": "",  # Empty file path
         "messageFaults": [],
     }
 
+    # Act
     result = await matlab_proxy_fixture._read_eval_response_from_file(eval_response)
 
+    # Assert
     # Verify empty result returns empty list
     assert result == []
 
@@ -360,13 +377,14 @@ async def test_read_eval_response_from_file_error_with_message_faults(
     matlab_proxy_fixture,
 ):
     """Test _read_eval_response_from_file with error response containing message faults."""
-
+    # Arrange
     eval_response = {
         "isError": True,
         "responseStr": "Error occurred",
         "messageFaults": [{"message": "Syntax error in code"}],
     }
 
+    # Act
     with pytest.raises(
         Exception,
         match="Failed to execute. Operation may have been interrupted by user.",

Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@`
`27`	`27`	`)`
`28`	`28`	`from jupyter_matlab_kernel.mwi_exceptions import MATLABConnectionError`
`29`	`29`
`30`		`-from jupyter_matlab_kernel.kernels.comms import LabExtensionCommunication`
	`30`	`+from jupyter_matlab_kernel.comms import LabExtensionCommunication`
`31`	`31`
`32`	`32`
`33`	`33`	`_MATLAB_STARTUP_TIMEOUT = mwi_settings.get_process_startup_timeout()`