Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove EOT from testing communication #24220

Merged
merged 6 commits into from
Oct 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions python_files/tests/pytestadapter/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
5 changes: 0 additions & 5 deletions python_files/unittestadapter/discovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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)
4 changes: 0 additions & 4 deletions python_files/unittestadapter/django_test_runner.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 = (
Expand Down
26 changes: 1 addition & 25 deletions python_files/unittestadapter/execution.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@

from unittestadapter.pvsc_utils import ( # noqa: E402
CoveragePayloadDict,
EOTPayloadDict,
ExecutionPayloadDict,
FileCoverageInfo,
TestExecutionStatus,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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(
Expand Down
9 changes: 1 addition & 8 deletions python_files/unittestadapter/pvsc_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -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],
):
"""
Expand Down
13 changes: 1 addition & 12 deletions python_files/vscode_pytest/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -873,7 +862,7 @@ def default(self, o):


def send_post_request(
payload: ExecutionPayloadDict | DiscoveryPayloadDict | EOTPayloadDict | CoveragePayloadDict,
payload: ExecutionPayloadDict | DiscoveryPayloadDict | CoveragePayloadDict,
cls_encoder=None,
):
"""
Expand Down
31 changes: 5 additions & 26 deletions src/client/testing/testController/common/resultResolver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
18 changes: 2 additions & 16 deletions src/client/testing/testController/common/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -259,11 +250,6 @@ export type DiscoveredTestPayload = {
error?: string[];
};

export type EOTTestPayload = {
commandType: 'discovery' | 'execution';
eot: boolean;
};

export type CoveragePayload = {
coverage: boolean;
cwd: string;
Expand Down
22 changes: 6 additions & 16 deletions src/client/testing/testController/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import {
DiscoveredTestItem,
DiscoveredTestNode,
DiscoveredTestPayload,
EOTTestPayload,
ExecutionTestPayload,
ITestResultResolver,
} from './types';
Expand Down Expand Up @@ -193,7 +192,7 @@ export async function startTestIdsNamedPipe(testIds: string[]): Promise<string>
}

interface ExecutionResultMessage extends Message {
params: ExecutionTestPayload | EOTTestPayload;
params: ExecutionTestPayload;
}

/**
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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(() => {
Expand All @@ -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');
Expand All @@ -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();
}),
Expand Down Expand Up @@ -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.
*
Expand Down
Loading
Loading