Branch remote runner plugin#23
Conversation
Set up the remote runner plugin, and associated directory and index files to be linked to the ev3 engine in py-slang
|
There was a problem hiding this comment.
Code Review
This pull request establishes a monorepo structure for plugins using Yarn workspaces, adding configuration files for ESLint, Prettier, Jest, and VSCode, alongside a custom build script to manage plugin manifests. It also bootstraps initial test packages for common, runner, and web environments. The code review highlights several critical issues that must be addressed before merging, including a missing type export (PySlangMessage), hardcoded absolute local file paths in imports and tsconfig.json mappings, incorrect import paths in tests, a hardcoded failing test assertion, and a class naming convention violation.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| export const CHANNEL_ID = "test"; | ||
|
|
||
| export type TestMessage = "ping" | "pong"; |
There was a problem hiding this comment.
The type PySlangMessage is imported by both src/runner/test/src/remoteExecution/index.ts and src/runner/test/src/__tests__/runner.test.ts from @sourceacademy/common-test, but it is not defined or exported here. This will cause compilation errors. Please define and export PySlangMessage in this file.
| export type TestMessage = "ping" | "pong"; | |
| export type TestMessage = "ping" | "pong"; | |
| export type PySlangMessage = { | |
| type: "run"; | |
| code: string; | |
| }; |
There was a problem hiding this comment.
++, but put it in a separate common/remoteExecution package
| @@ -0,0 +1,26 @@ | |||
| import { CHANNEL_ID, RUNNER_ID, type PySlangMessage } from "@sourceacademy/common-test"; | |||
| import { IPlugin, IChannel, IConduit } from "@sourceacademy/conductor/conduit"; | |||
| import { EV3Engine } from "C:/Users/Migue/OneDrive/Desktop/Projects/py-slang-2/src/engines/ev3/EV3Engine"; | |||
There was a problem hiding this comment.
Avoid using absolute local file paths (e.g., C:/Users/...) for imports, as this will fail to compile on any other machine or in CI/CD environments. Since py-slang is listed as a dependency in package.json, you should import EV3Engine directly from the py-slang package.
| import { EV3Engine } from "C:/Users/Migue/OneDrive/Desktop/Projects/py-slang-2/src/engines/ev3/EV3Engine"; | |
| import { EV3Engine } from "py-slang"; |
| "rootDir": ".", | ||
| "paths": { | ||
| "@py-slang/*": ["C:/Users/Migue/OneDrive/Desktop/Projects/py-slang-2/src/*"] | ||
| } |
| @@ -0,0 +1,25 @@ | |||
| import { remoteRunnerPlugin } from ".."; | |||
There was a problem hiding this comment.
The import of remoteRunnerPlugin from ".." (which points to src/runner/test/src/index.ts) will fail because remoteRunnerPlugin is not exported from the package entry point. Import it directly from the relative path ../remoteExecution to fix the test compilation, and consider exporting it from src/runner/test/src/index.ts if it is intended to be part of the public API.
| import { remoteRunnerPlugin } from ".."; | |
| import { remoteRunnerPlugin } from "../remoteExecution"; |
|
|
||
| expect(sentMessages.length).toBeGreaterThan(0); | ||
| console.log(sentMessages[0]); | ||
| expect(sentMessages[0]).toEqual("FORCE_FAIL_TO_SEE_OUTPUT"); |
There was a problem hiding this comment.
The assertion expect(sentMessages[0]).toEqual("FORCE_FAIL_TO_SEE_OUTPUT"); is hardcoded to fail. This was likely added for debugging purposes to inspect the output. Please replace it with a proper assertion matching the expected output of the engine execution so that the test suite can pass in CI.
| expect(sentMessages[0]).toEqual("FORCE_FAIL_TO_SEE_OUTPUT"); | |
| expect(sentMessages[0]).toBeDefined(); |
| import { IPlugin, IChannel, IConduit } from "@sourceacademy/conductor/conduit"; | ||
| import { EV3Engine } from "C:/Users/Migue/OneDrive/Desktop/Projects/py-slang-2/src/engines/ev3/EV3Engine"; | ||
|
|
||
| export class remoteRunnerPlugin implements IPlugin { |
There was a problem hiding this comment.
Following standard TypeScript naming conventions, class names should use PascalCase. Please rename remoteRunnerPlugin to RemoteRunnerPlugin and update all references accordingly.
| export class remoteRunnerPlugin implements IPlugin { | |
| export class RemoteRunnerPlugin implements IPlugin { |
|
Could you please rebase the branch off |
AaravMalani
left a comment
There was a problem hiding this comment.
I may have missed a couple of changes, but for now, I have a couple of recommendations
- Rebase the branch onto main as mentioned in the previous comments (it'll help me look through your code easily as well)
- Move the remote execution code into a separate folder. Each directory under the
runner,commonandwebrepresents a distinct package. They'll each have their ownpackage.json, manifest, etc. and get published as a separate package (if they're installable) or deployed onto GitHub Pages under a separate plugin definition in the plugin directory (if they're external)
There was a problem hiding this comment.
The remoteExecution directory would be a sibling of the runner/test directory, not a child of it.
runner
├─── test
└─── remoteExecution
It would have its own manifest, package.json, rollup.config.mjs, etc.
Just make a copy of the runner/test directory and rename it to runner/remoteExecution.
| import { IPlugin, IChannel, IConduit } from "@sourceacademy/conductor/conduit"; | ||
| import { EV3Engine } from "C:/Users/Migue/OneDrive/Desktop/Projects/py-slang-2/src/engines/ev3/EV3Engine"; | ||
|
|
||
| export class remoteRunnerPlugin implements IPlugin { |
| @@ -0,0 +1,26 @@ | |||
| import { CHANNEL_ID, RUNNER_ID, type PySlangMessage } from "@sourceacademy/common-test"; | |||
| import { IPlugin, IChannel, IConduit } from "@sourceacademy/conductor/conduit"; | |||
| import { EV3Engine } from "C:/Users/Migue/OneDrive/Desktop/Projects/py-slang-2/src/engines/ev3/EV3Engine"; | |||
|
|
||
| constructor( | ||
| _conduit: IConduit, | ||
| [channel]: IChannel<any>[], |
There was a problem hiding this comment.
You might have to ESLint ignore this line for now, otherwise it throws a warning
| @@ -0,0 +1,26 @@ | |||
| import { CHANNEL_ID, RUNNER_ID, type PySlangMessage } from "@sourceacademy/common-test"; | |||
There was a problem hiding this comment.
It shouldn't share the same CHANNEL_ID and RUNNER_ID as the test package
They are separate plugins
|
|
||
| export const CHANNEL_ID = "test"; | ||
|
|
||
| export type TestMessage = "ping" | "pong"; |
There was a problem hiding this comment.
++, but put it in a separate common/remoteExecution package
Set up a remote runner plugin under the runner/dist directory to receive code from the ev3 engine in py-slang