Skip to content

feat: added executor tests #1816

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.

Sign up for GitHub

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

Merged
RohitKushvaha01 merged 8 commits into from
Jan 16, 2026
Merged

feat: added executor tests #1816

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3403337
feat: added executor tests
RohitKushvaha01 Jan 13, 2026
2edc3db
Update src/test/exec.tests.js
RohitKushvaha01 Jan 13, 2026
3502ba5
feat: added executor tests
RohitKushvaha01 Jan 13, 2026
3bedaa2
feat: added delay
RohitKushvaha01 Jan 13, 2026
d332a98
feat: added missing import
RohitKushvaha01 Jan 13, 2026
9dd7c25
feat: removed missing import
RohitKushvaha01 Jan 13, 2026
6bf32ce
fix: tests
RohitKushvaha01 Jan 16, 2026
b4cd1b3
format
RohitKushvaha01 Jan 16, 2026
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
179 changes: 179 additions & 0 deletions src/test/exec.tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,179 @@
import { TestRunner } from "./tester";

export async function runExecutorTests(writeOutput) {
const runner = new TestRunner("Executor API Tests");

runner.test("Executor available", async (test) => {
test.assert(typeof Executor !== "undefined", "Executor should be available globally");
});

runner.test("Background Executor available", async (test) => {
test.assert(typeof Executor.BackgroundExecutor !== "undefined", "Background Executor should be available globally");
});


runner.test("execute()", async (test) => {

test.assert(
await Executor.execute("echo test123").includes("test123"),
"Command output should match"
);

});



runner.test("execute() (BackgroundExecutor)", async (test) => {

test.assert(
await Executor.BackgroundExecutor.execute("echo test123").includes("test123"),
"Command output should match"
);

});

runner.test("start()", async (test) => {
let stdout = "";

const uuid = await Executor.start("sh", (type, data) => {
if (type === "stdout") stdout += data;
});

await Executor.write(uuid, "echo hello\n");

await Executor.stop(uuid);

await new Promise(r => setTimeout(r, 200));

test.assert(stdout.includes("hello"), "Shell should echo output");
});
Comment thread
RohitKushvaha01 marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential timing issue: The test stops the executor at line 44, then waits 200ms at line 46, and then checks the output at line 48. However, the callback on line 38-40 may receive output asynchronously after the stop() call. This test might pass or fail intermittently depending on timing.

Consider waiting before calling stop() to ensure all output has been received, or add an exit event handler to the callback to know when the process has truly finished.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/exec.tests.js
Line: 35:49

Comment:
Potential timing issue: The test stops the executor at line 44, then waits 200ms at line 46, and then checks the output at line 48. However, the callback on line 38-40 may receive output asynchronously after the stop() call. This test might pass or fail intermittently depending on timing.

Consider waiting before calling stop() to ensure all output has been received, or add an exit event handler to the callback to know when the process has truly finished.

How can I resolve this? If you propose a fix, please make it concise.



runner.test("start() (BackgroundExecutor)", async (test) => {
let stdout = "";

const uuid = await Executor.start("sh", (type, data) => {
if (type === "stdout") stdout += data;
});

await Executor.write(uuid, "echo hello\n");
await Executor.stop(uuid);
Comment thread
RohitKushvaha01 marked this conversation as resolved.
Outdated

await new Promise(r => setTimeout(r, 200));

test.assert(stdout.includes("hello"), "Shell should echo output");
});


runner.test("start/stop()", async (test) => {
let stdout = "";

const uuid = await Executor.start("sh", (type, data) => {

});

await new Promise(r => setTimeout(r, 200));



const isRunning = await Executor.isRunning(uuid);

test.assert(isRunning === true, "Executor must be running");


await new Promise(r => setTimeout(r, 200));


await Executor.stop(uuid);

await new Promise(r => setTimeout(r, 200));


test.assert(isRunning !== await Executor.isRunning(uuid), "Executor must be stopped");
Comment thread
RohitKushvaha01 marked this conversation as resolved.
Outdated
test.assert(await Executor.isRunning(uuid) === false, "Executor must be stopped");
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: isRunning is captured at line 79 (before stop), then the executor is stopped at line 87, and finally line 92 checks if isRunning !== await Executor.isRunning(uuid). This assertion will always pass because it compares the old value (true) with the new value (false), but this doesn't actually verify that the stop operation worked correctly - it only verifies that the value changed.

The logic on line 92 is redundant with line 93, which properly checks the current state. Line 92 should be removed, or the test should be restructured to properly verify both running and stopped states separately.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/exec.tests.js
Line: 79:93

Comment:
Race condition: `isRunning` is captured at line 79 (before stop), then the executor is stopped at line 87, and finally line 92 checks if `isRunning !== await Executor.isRunning(uuid)`. This assertion will always pass because it compares the old value (true) with the new value (false), but this doesn't actually verify that the stop operation worked correctly - it only verifies that the value changed.

The logic on line 92 is redundant with line 93, which properly checks the current state. Line 92 should be removed, or the test should be restructured to properly verify both running and stopped states separately.

How can I resolve this? If you propose a fix, please make it concise.


});
Comment thread
RohitKushvaha01 marked this conversation as resolved.
Outdated

runner.test("start/stop() (BackgroundExecutor)", async (test) => {
let stdout = "";

const uuid = await Executor.start("sh", (type, data) => {

});

await new Promise(r => setTimeout(r, 200));



const isRunning = await Executor.isRunning(uuid);

test.assert(isRunning === true, "Executor must be running");


await new Promise(r => setTimeout(r, 200));


await Executor.stop(uuid);

await new Promise(r => setTimeout(r, 200));


test.assert(isRunning !== await Executor.isRunning(uuid), "Executor must be stopped");
test.assert(await Executor.isRunning(uuid) === false, "Executor must be stopped");
Comment thread
RohitKushvaha01 marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: Same issue as in the previous start/stop() test. isRunning is captured at line 108 (before stop), then the executor is stopped at line 116, and line 121 checks if isRunning !== await Executor.isRunning(uuid). This assertion will always pass but doesn't properly verify the stop operation worked - it only verifies that the value changed.

Line 121 is redundant with line 122, which properly checks the current state. Line 121 should be removed.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/exec.tests.js
Line: 108:122

Comment:
Race condition: Same issue as in the previous start/stop() test. `isRunning` is captured at line 108 (before stop), then the executor is stopped at line 116, and line 121 checks if `isRunning !== await Executor.isRunning(uuid)`. This assertion will always pass but doesn't properly verify the stop operation worked - it only verifies that the value changed.

Line 121 is redundant with line 122, which properly checks the current state. Line 121 should be removed.

How can I resolve this? If you propose a fix, please make it concise.


});
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is labeled "start/stop() (BackgroundExecutor)" but it's actually testing the regular Executor, not BackgroundExecutor. Lines 100, 108, 116, 121, and 122 all use Executor.start(), Executor.isRunning(), and Executor.stop() instead of Executor.BackgroundExecutor.* methods.

To properly test BackgroundExecutor, these calls should use Executor.BackgroundExecutor like the execute() test does on line 29.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/exec.tests.js
Line: 97:124

Comment:
This test is labeled "start/stop() (BackgroundExecutor)" but it's actually testing the regular Executor, not BackgroundExecutor. Lines 100, 108, 116, 121, and 122 all use `Executor.start()`, `Executor.isRunning()`, and `Executor.stop()` instead of `Executor.BackgroundExecutor.*` methods.

To properly test BackgroundExecutor, these calls should use `Executor.BackgroundExecutor` like the execute() test does on line 29.

How can I resolve this? If you propose a fix, please make it concise.







runner.test("start/stop()", async (test) => {
let stdout = "";

const uuid = await Executor.start("sh", (type, data) => {

});

await new Promise(r => setTimeout(r, 200));



const isRunning = await Executor.isRunning(uuid);

test.assert(isRunning === true, "Executor must be running");


await new Promise(r => setTimeout(r, 200));


await Executor.stop(uuid);

await new Promise(r => setTimeout(r, 200));


test.assert(isRunning !== await Executor.isRunning(uuid), "Executor must be stopped");
test.assert(await Executor.isRunning(uuid) === false, "Executor must be stopped");
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race condition: Same issue as in the other start/stop() tests. isRunning is captured at line 142 (before stop), then the executor is stopped at line 150, and line 155 checks if isRunning !== await Executor.isRunning(uuid). This assertion will always pass but doesn't properly verify the stop operation.

Line 155 is redundant with line 156. Line 155 should be removed.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/exec.tests.js
Line: 142:156

Comment:
Race condition: Same issue as in the other start/stop() tests. `isRunning` is captured at line 142 (before stop), then the executor is stopped at line 150, and line 155 checks if `isRunning !== await Executor.isRunning(uuid)`. This assertion will always pass but doesn't properly verify the stop operation.

Line 155 is redundant with line 156. Line 155 should be removed.

How can I resolve this? If you propose a fix, please make it concise.


});
Comment thread
RohitKushvaha01 marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate test name "start/stop()". This test has the exact same name as the test on line 68, which will cause issues with test reporting and make it difficult to identify which test failed. This should have a unique name like "start/stop() lifecycle" or be removed if it's an accidental duplicate.

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/exec.tests.js
Line: 131:158

Comment:
Duplicate test name "start/stop()". This test has the exact same name as the test on line 68, which will cause issues with test reporting and make it difficult to identify which test failed. This should have a unique name like "start/stop() lifecycle" or be removed if it's an accidental duplicate.

How can I resolve this? If you propose a fix, please make it concise.


runner.test("FDROID env variable", async (test) => {
const result = await Executor.execute("echo $FDROID");

const isSet = result.trim().length > 0;

test.assert(
isSet,
"FDROID env variable should be set"
);
});
Copy link
Copy Markdown
Contributor

@greptile-apps greptile-apps bot Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Environment-dependent test: This test assumes the FDROID environment variable is always set and has a non-empty value. However, this may not be true in all test environments (e.g., local development, CI/CD pipelines, non-F-Droid builds).

Consider using test.skip() if the environment variable isn't set, or adjust the assertion to handle cases where the variable legitimately might not exist: test.assert(typeof result === 'string', "FDROID env variable should be accessible")

Prompt To Fix With AI 
This is a comment left during a code review.
Path: src/test/exec.tests.js
Line: 160:169

Comment:
Environment-dependent test: This test assumes the FDROID environment variable is always set and has a non-empty value. However, this may not be true in all test environments (e.g., local development, CI/CD pipelines, non-F-Droid builds).

Consider using `test.skip()` if the environment variable isn't set, or adjust the assertion to handle cases where the variable legitimately might not exist: `test.assert(typeof result === 'string', "FDROID env variable should be accessible")`

How can I resolve this? If you propose a fix, please make it concise.









return await runner.run(writeOutput);
}
81 changes: 61 additions & 20 deletions src/test/tester.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export async function runAllTests() {
// Run unit tests
await runSanityTests(write);
await runAceEditorTests(write);
await runExecutorTests(write);

write("\x1b[36m\x1b[1mTests completed!\x1b[0m\n");
} catch (error) {
Expand Down Expand Up @@ -80,6 +81,7 @@ class TestRunner {
this.passed = 0;
this.failed = 0;
this.results = [];
this.skipped = 0;
}

/**
Expand All @@ -104,6 +106,11 @@ class TestRunner {
}
}

skip(reason = "Skipped") {
throw new SkipTest(reason);
}


async _runWithTimeout(fn, ctx, timeoutMs) {
return new Promise((resolve, reject) => {
let finished = false;
Expand Down Expand Up @@ -161,48 +168,73 @@ class TestRunner {

try {
await delay(200);

await this._runWithTimeout(test.fn, this, 3000);

stopSpinner();

this.passed++;
this.results.push({ name: test.name, status: "PASS", error: null });
this.results.push({ name: test.name, status: "PASS" });
line(` ${COLORS.GREEN}✓${COLORS.RESET} ${test.name}`, COLORS.GREEN);

} catch (error) {
stopSpinner();

this.failed++;
this.results.push({
name: test.name,
status: "FAIL",
error: error.message,
});
line(
` ${COLORS.RED}✗${COLORS.RESET} ${test.name}`,
COLORS.RED + COLORS.BRIGHT,
);
line(
` ${COLORS.DIM}└─ ${error.message}${COLORS.RESET}`,
COLORS.RED + COLORS.DIM,
);
if (error instanceof SkipTest) {
this.skipped++;
this.results.push({
name: test.name,
status: "SKIP",
reason: error.message,
});

line(
` ${COLORS.YELLOW}?${COLORS.RESET} ${test.name}`,
COLORS.YELLOW + COLORS.BRIGHT,
);
line(
` ${COLORS.DIM}└─ ${error.message}${COLORS.RESET}`,
COLORS.YELLOW + COLORS.DIM,
);
} else {
this.failed++;
this.results.push({
name: test.name,
status: "FAIL",
error: error.message,
});

line(
` ${COLORS.RED}✗${COLORS.RESET} ${test.name}`,
COLORS.RED + COLORS.BRIGHT,
);
line(
` ${COLORS.DIM}└─ ${error.message}${COLORS.RESET}`,
COLORS.RED + COLORS.DIM,
);
}
}

}

// Summary
line();
line("─────────────────────────────────────────────", COLORS.GRAY);

const total = this.tests.length;
const percentage = total ? ((this.passed / total) * 100).toFixed(1) : "0.0";
const effectiveTotal = total - this.skipped;

const percentage = effectiveTotal
? ((this.passed / effectiveTotal) * 100).toFixed(1)
: "0.0";


const statusColor = this.failed === 0 ? COLORS.GREEN : COLORS.YELLOW;

line(
` Tests: ${COLORS.BRIGHT}${total}${COLORS.RESET} | ` +
`${statusColor}Passed: ${this.passed}${COLORS.RESET} | ` +
`${COLORS.RED}Failed: ${this.failed}${COLORS.RESET}`,
statusColor,
`${COLORS.GREEN}Passed: ${this.passed}${COLORS.RESET} | ` +
`${COLORS.YELLOW}Skipped: ${this.skipped}${COLORS.RESET} | ` +
`${COLORS.RED}Failed: ${this.failed}${COLORS.RESET}`,
);

line(
Expand All @@ -226,4 +258,13 @@ class TestRunner {
}
}


class SkipTest extends Error {
constructor(message = "Skipped") {
super(message);
this.name = "SkipTest";
}
}


export { TestRunner };
Loading