remove EOT from testing communication (#24220)

remove the need for the EOT in the communication design between the extension and the python subprocesses produced to run testing.
microsoft · Oct 7, 2024 · 7513198 · 7513198
1 parent a73eb9c
commit 7513198
Show file tree

Hide file tree

Showing 18 changed files with 64 additions and 283 deletions.
diff --git a/python_files/tests/pytestadapter/helpers.py b/python_files/tests/pytestadapter/helpers.py
@@ -71,7 +71,6 @@ def process_data_received(data: str) -> List[Dict[str, Any]]:
 
     This function also:
     - Checks that the jsonrpc value is 2.0
-    - Checks that the last JSON message contains the `eot` token.
     """
     json_messages = []
     remaining = data
@@ -85,18 +84,14 @@ def process_data_received(data: str) -> List[Dict[str, Any]]:
         else:
             json_messages.append(json_data["params"])
 
-    last_json = json_messages.pop(-1)
-    if "eot" not in last_json:
-        raise ValueError("Last JSON messages does not contain 'eot' as its last payload.")
-    return json_messages  # return the list of json messages, only the params part without the EOT token
+    return json_messages  # return the list of json messages
 
 
 def parse_rpc_message(data: str) -> Tuple[Dict[str, str], str]:
     """Process the JSON data which comes from the server.
 
     A single rpc payload is in the format:
     content-length: #LEN# \r\ncontent-type: application/json\r\n\r\n{"jsonrpc": "2.0", "params": ENTIRE_DATA}
-    with EOT params: "params": {"command_type": "discovery", "eot": true}
 
     returns:
     json_data: A single rpc payload of JSON data from the server.

diff --git a/python_files/unittestadapter/discovery.py b/python_files/unittestadapter/discovery.py
@@ -16,7 +16,6 @@
 # If I use from utils then there will be an import error in test_discovery.py.
 from unittestadapter.pvsc_utils import (  # noqa: E402
     DiscoveryPayloadDict,
-    EOTPayloadDict,
     VSCodeUnittestError,
     build_test_tree,
     parse_unittest_args,
@@ -129,7 +128,6 @@ def discover_tests(
             # collect args for Django discovery runner.
             args = argv[index + 1 :] or []
             django_discovery_runner(manage_py_path, args)
-            # eot payload sent within Django runner.
         except Exception as e:
             error_msg = f"Error configuring Django test runner: {e}"
             print(error_msg, file=sys.stderr)
@@ -139,6 +137,3 @@ def discover_tests(
         payload = discover_tests(start_dir, pattern, top_level_dir)
         # Post this discovery payload.
         send_post_request(payload, test_run_pipe)
-        # Post EOT token.
-        eot_payload: EOTPayloadDict = {"command_type": "discovery", "eot": True}
-        send_post_request(eot_payload, test_run_pipe)
diff --git a/python_files/unittestadapter/django_test_runner.py b/python_files/unittestadapter/django_test_runner.py
@@ -13,7 +13,6 @@
 from execution import UnittestTestResult  # noqa: E402
 from pvsc_utils import (  # noqa: E402
     DiscoveryPayloadDict,
-    EOTPayloadDict,
     VSCodeUnittestError,
     build_test_tree,
     send_post_request,
@@ -64,9 +63,6 @@ def run_tests(self, test_labels, **kwargs):
 
             # Send discovery payload.
             send_post_request(payload, test_run_pipe)
-            # Send EOT token.
-            eot_payload: EOTPayloadDict = {"command_type": "discovery", "eot": True}
-            send_post_request(eot_payload, test_run_pipe)
             return 0  # Skip actual test execution, return 0 as no tests were run.
         except Exception as e:
             error_msg = (

diff --git a/python_files/unittestadapter/execution.py b/python_files/unittestadapter/execution.py
@@ -25,7 +25,6 @@
 
 from unittestadapter.pvsc_utils import (  # noqa: E402
     CoveragePayloadDict,
-    EOTPayloadDict,
     ExecutionPayloadDict,
     FileCoverageInfo,
     TestExecutionStatus,
@@ -60,21 +59,6 @@ def startTest(self, test: unittest.TestCase):  # noqa: N802
 
     def stopTestRun(self):  # noqa: N802
         super().stopTestRun()
-        # After stopping the test run, send EOT
-        test_run_pipe = os.getenv("TEST_RUN_PIPE")
-        if os.getenv("MANAGE_PY_PATH"):
-            # only send this if it is a Django run
-            if not test_run_pipe:
-                print(
-                    "UNITTEST ERROR: TEST_RUN_PIPE is not set at the time of unittest trying to send data. "
-                    f"TEST_RUN_PIPE = {test_run_pipe}\n",
-                    file=sys.stderr,
-                )
-                raise VSCodeUnittestError(
-                    "UNITTEST ERROR: TEST_RUN_PIPE is not set at the time of unittest trying to send data. "
-                )
-            eot_payload: EOTPayloadDict = {"command_type": "execution", "eot": True}
-            send_post_request(eot_payload, test_run_pipe)
 
     def addError(  # noqa: N802
         self,
@@ -269,15 +253,8 @@ def run_tests(
     return payload
 
 
-def execute_eot_and_cleanup():
-    eot_payload: EOTPayloadDict = {"command_type": "execution", "eot": True}
-    send_post_request(eot_payload, test_run_pipe)
-    if __socket:
-        __socket.close()
-
-
 __socket = None
-atexit.register(execute_eot_and_cleanup)
+atexit.register(lambda: __socket.close() if __socket else None)
 
 
 def send_run_data(raw_data, test_run_pipe):
@@ -361,7 +338,6 @@ def send_run_data(raw_data, test_run_pipe):
         print("MANAGE_PY_PATH env var set, running Django test suite.")
         args = argv[index + 1 :] or []
         django_execution_runner(manage_py_path, test_ids, args)
-        # the django run subprocesses sends the eot payload.
     else:
         # Perform regular unittest execution.
         payload = run_tests(

diff --git a/python_files/unittestadapter/pvsc_utils.py b/python_files/unittestadapter/pvsc_utils.py
@@ -74,13 +74,6 @@ class ExecutionPayloadDict(TypedDict):
     error: NotRequired[str]
 
 
-class EOTPayloadDict(TypedDict):
-    """A dictionary that is used to send a end of transmission post request to the server."""
-
-    command_type: Literal["discovery", "execution"]
-    eot: bool
-
-
 class FileCoverageInfo(TypedDict):
     lines_covered: List[int]
     lines_missed: List[int]
@@ -314,7 +307,7 @@ def parse_unittest_args(
 
 
 def send_post_request(
-    payload: Union[ExecutionPayloadDict, DiscoveryPayloadDict, EOTPayloadDict, CoveragePayloadDict],
+    payload: Union[ExecutionPayloadDict, DiscoveryPayloadDict, CoveragePayloadDict],
     test_run_pipe: Optional[str],
 ):
     """

diff --git a/python_files/vscode_pytest/__init__.py b/python_files/vscode_pytest/__init__.py
@@ -455,10 +455,6 @@ def pytest_sessionfinish(session, exitstatus):
         )
         send_post_request(payload)
 
-    command_type = "discovery" if IS_DISCOVERY else "execution"
-    payload_eot: EOTPayloadDict = {"command_type": command_type, "eot": True}
-    send_post_request(payload_eot)
-
 
 def build_test_tree(session: pytest.Session) -> TestNode:
     """Builds a tree made up of testing nodes from the pytest session.
@@ -782,13 +778,6 @@ class CoveragePayloadDict(Dict):
     error: str | None  # Currently unused need to check
 
 
-class EOTPayloadDict(TypedDict):
-    """A dictionary that is used to send a end of transmission post request to the server."""
-
-    command_type: Literal["discovery", "execution"]
-    eot: bool
-
-
 def get_node_path(node: Any) -> pathlib.Path:
     """A function that returns the path of a node given the switch to pathlib.Path.
 
@@ -873,7 +862,7 @@ def default(self, o):
 
 
 def send_post_request(
-    payload: ExecutionPayloadDict | DiscoveryPayloadDict | EOTPayloadDict | CoveragePayloadDict,
+    payload: ExecutionPayloadDict | DiscoveryPayloadDict | CoveragePayloadDict,
     cls_encoder=None,
 ):
     """

diff --git a/src/client/testing/testController/common/resultResolver.ts b/src/client/testing/testController/common/resultResolver.ts
@@ -17,13 +17,7 @@ import {
     Range,
 } from 'vscode';
 import * as util from 'util';
-import {
-    CoveragePayload,
-    DiscoveredTestPayload,
-    EOTTestPayload,
-    ExecutionTestPayload,
-    ITestResultResolver,
-} from './types';
+import { CoveragePayload, DiscoveredTestPayload, ExecutionTestPayload, ITestResultResolver } from './types';
 import { TestProvider } from '../../types';
 import { traceError, traceVerbose } from '../../../logging';
 import { Testing } from '../../../common/utils/localize';
@@ -32,7 +26,6 @@ import { sendTelemetryEvent } from '../../../telemetry';
 import { EventName } from '../../../telemetry/constants';
 import { splitLines } from '../../../common/stringUtils';
 import { buildErrorNodeOptions, populateTestTree, splitTestNameWithRegex } from './utils';
-import { Deferred } from '../../../common/utils/async';
 
 export class PythonResultResolver implements ITestResultResolver {
     testController: TestController;
@@ -58,14 +51,8 @@ export class PythonResultResolver implements ITestResultResolver {
         this.vsIdToRunId = new Map<string, string>();
     }
 
-    public resolveDiscovery(
-        payload: DiscoveredTestPayload | EOTTestPayload,
-        deferredTillEOT: Deferred<void>,
-        token?: CancellationToken,
-    ): void {
-        if ('eot' in payload && payload.eot === true) {
-            deferredTillEOT.resolve();
-        } else if (!payload) {
+    public resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void {
+        if (!payload) {
             // No test data is available
         } else {
             this._resolveDiscovery(payload as DiscoveredTestPayload, token);
@@ -117,16 +104,8 @@ export class PythonResultResolver implements ITestResultResolver {
         });
     }
 
-    public resolveExecution(
-        payload: ExecutionTestPayload | EOTTestPayload | CoveragePayload,
-        runInstance: TestRun,
-        deferredTillEOT: Deferred<void>,
-    ): void {
-        if ('eot' in payload && payload.eot === true) {
-            // eot sent once per connection
-            traceVerbose('EOT received, resolving deferredTillServerClose');
-            deferredTillEOT.resolve();
-        } else if ('coverage' in payload) {
+    public resolveExecution(payload: ExecutionTestPayload | CoveragePayload, runInstance: TestRun): void {
+        if ('coverage' in payload) {
             // coverage data is sent once per connection
             traceVerbose('Coverage data received.');
             this._resolveCoverage(payload as CoveragePayload, runInstance);

diff --git a/src/client/testing/testController/common/types.ts b/src/client/testing/testController/common/types.ts
@@ -16,7 +16,6 @@ import {
 import { ITestDebugLauncher, TestDiscoveryOptions } from '../../common/types';
 import { IPythonExecutionFactory } from '../../../common/process/types';
 import { EnvironmentVariables } from '../../../common/variables/types';
-import { Deferred } from '../../../common/utils/async';
 
 export type TestRunInstanceOptions = TestRunOptions & {
     exclude?: readonly TestItem[];
@@ -198,16 +197,8 @@ export interface ITestResultResolver {
     vsIdToRunId: Map<string, string>;
     detailedCoverageMap: Map<string, FileCoverageDetail[]>;
 
-    resolveDiscovery(
-        payload: DiscoveredTestPayload | EOTTestPayload,
-        deferredTillEOT: Deferred<void>,
-        token?: CancellationToken,
-    ): void;
-    resolveExecution(
-        payload: ExecutionTestPayload | EOTTestPayload | CoveragePayload,
-        runInstance: TestRun,
-        deferredTillEOT: Deferred<void>,
-    ): void;
+    resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void;
+    resolveExecution(payload: ExecutionTestPayload | CoveragePayload, runInstance: TestRun): void;
     _resolveDiscovery(payload: DiscoveredTestPayload, token?: CancellationToken): void;
     _resolveExecution(payload: ExecutionTestPayload, runInstance: TestRun): void;
     _resolveCoverage(payload: CoveragePayload, runInstance: TestRun): void;
@@ -259,11 +250,6 @@ export type DiscoveredTestPayload = {
     error?: string[];
 };
 
-export type EOTTestPayload = {
-    commandType: 'discovery' | 'execution';
-    eot: boolean;
-};
-
 export type CoveragePayload = {
     coverage: boolean;
     cwd: string;

diff --git a/src/client/testing/testController/common/utils.ts b/src/client/testing/testController/common/utils.ts
@@ -16,7 +16,6 @@ import {
     DiscoveredTestItem,
     DiscoveredTestNode,
     DiscoveredTestPayload,
-    EOTTestPayload,
     ExecutionTestPayload,
     ITestResultResolver,
 } from './types';
@@ -193,7 +192,7 @@ export async function startTestIdsNamedPipe(testIds: string[]): Promise<string>
 }
 
 interface ExecutionResultMessage extends Message {
-    params: ExecutionTestPayload | EOTTestPayload;
+    params: ExecutionTestPayload;
 }
 
 /**
@@ -227,7 +226,7 @@ export async function writeTestIdsFile(testIds: string[]): Promise<string> {
 }
 
 export async function startRunResultNamedPipe(
-    dataReceivedCallback: (payload: ExecutionTestPayload | EOTTestPayload) => void,
+    dataReceivedCallback: (payload: ExecutionTestPayload) => void,
     deferredTillServerClose: Deferred<void>,
     cancellationToken?: CancellationToken,
 ): Promise<{ name: string } & Disposable> {
@@ -259,8 +258,7 @@ export async function startRunResultNamedPipe(
             }),
             reader.listen((data: Message) => {
                 traceVerbose(`Test Result named pipe ${pipeName} received data`);
-                // if EOT, call decrement connection count (callback)
-                dataReceivedCallback((data as ExecutionResultMessage).params as ExecutionTestPayload | EOTTestPayload);
+                dataReceivedCallback((data as ExecutionResultMessage).params as ExecutionTestPayload);
             }),
         );
         server.serverOnClosePromise().then(() => {
@@ -275,11 +273,11 @@ export async function startRunResultNamedPipe(
 }
 
 interface DiscoveryResultMessage extends Message {
-    params: DiscoveredTestPayload | EOTTestPayload;
+    params: DiscoveredTestPayload;
 }
 
 export async function startDiscoveryNamedPipe(
-    callback: (payload: DiscoveredTestPayload | EOTTestPayload) => void,
+    callback: (payload: DiscoveredTestPayload) => void,
     cancellationToken?: CancellationToken,
 ): Promise<{ name: string } & Disposable> {
     traceVerbose('Starting Test Discovery named pipe');
@@ -302,10 +300,9 @@ export async function startDiscoveryNamedPipe(
             }),
             reader.listen((data: Message) => {
                 traceVerbose(`Test Discovery named pipe ${pipeName} received data`);
-                callback((data as DiscoveryResultMessage).params as DiscoveredTestPayload | EOTTestPayload);
+                callback((data as DiscoveryResultMessage).params as DiscoveredTestPayload);
             }),
             reader.onClose(() => {
-                callback(createEOTPayload(true));
                 traceVerbose(`Test Discovery named pipe ${pipeName} closed`);
                 dispose();
             }),
@@ -475,13 +472,6 @@ export function createDiscoveryErrorPayload(
     };
 }
 
-export function createEOTPayload(executionBool: boolean): EOTTestPayload {
-    return {
-        commandType: executionBool ? 'execution' : 'discovery',
-        eot: true,
-    } as EOTTestPayload;
-}
-
 /**
  * Splits a test name into its parent test name and subtest unique section.
  *