Add automated tests, CI pipeline, and documentation improvements by JaivPatel07 · Pull Request #30 · DhanushNehru/OpenDrop

JaivPatel07 · 2026-03-20T05:58:24Z

Add tests, CI workflow, and improve README

What’s included

This PR adds proper test coverage, sets up CI, and improves the project documentation.

Testing

Added tests for the main functionality using Vitest and Supertest.

Covered:

Health check endpoint
File upload (single and batch)
File download
WebSocket behavior:
- Connection init
- Peer join / leave
- Signaling (offer, answer, candidate)

Also handled async cases and made sure WebSocket connections are cleaned up properly.

CI setup

Added a GitHub Actions workflow that:

Runs on push and pull requests
Installs dependencies
Runs all tests

This helps catch issues early before merging.

README updates

Added a "Running tests" section

Notes

Tests use port 4000
Make sure the port is free when running locally
There is a small update in index.js to support testing. The server start is now controlled so it does not run immediately during tests.

…est suite

Copilot

Pull request overview

This PR introduces an automated testing setup for the Node.js signaling server, adds a CI workflow to run those tests on pushes/PRs, and updates documentation with test-running instructions.

Changes:

Added Vitest + Supertest integration tests covering health, upload/download, and WebSocket signaling flows.
Added a GitHub Actions workflow to install server deps and run the test suite.
Updated server startup behavior and README to support/describe running tests.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`server/test/index.test.js`	New integration test suite for HTTP endpoints and WebSocket signaling.
`server/index.js`	Avoids auto-listening in tests and attempts to export `app`/`server` for test harness usage.
`server/package.json`	Adds Vitest/Supertest, test script, and switches module type.
`server/package-lock.json`	Locks newly added dev dependencies.
`README.md`	Adds “Running Tests” section and instructions.
`.github/workflows/test.yml`	New CI job to run server tests on push/PR.

Files not reviewed (1)

server/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

server/index.js

server/test/index.test.js

Copilot · 2026-03-21T12:47:41Z

server/test/index.test.js

+    const timeout = setTimeout(() => {
+      client.off('error', onError);
+      client.off('message', onMessage);
+      reject(new Error('Timed out waiting for init message'));
+    }, 3000);
+
+    const onError = (err) => {
+      clearTimeout(timeout);
+      client.off('message', onMessage);
+      reject(err);
+    };


On timeout, connectAndInitClient rejects but leaves the WebSocket open (no terminate/close), which can leak sockets and keep the test process alive on failures. Ensure the timeout/error paths terminate the client and remove listeners before rejecting.

Copilot · 2026-03-21T12:47:41Z

README.md

+### Prerequisites
+
+- Node.js 16+ and npm installed
+- Server dependencies installed in the `server` folder (`npm install`)
+
+### Commands
+
+From the project root:
+
+```bash
+cd server
+npx vitest run
+```
+
+Or run the npm test script (watch mode):
+
+```bash
+cd server
+npm test
+```


README says Node.js 16+ and describes npm test as "watch mode", but server/package.json runs vitest run and the added devDependencies (vitest/vite) require a newer Node version. Please update the Node version requirement and adjust the command descriptions to match the actual scripts (or update scripts to match the docs).

.github/workflows/test.yml

Copilot · 2026-03-21T12:47:41Z

server/test/index.test.js

+  it('should upload file and return file id', async () => {
+    const res = await request(baseURL)
+      .post('/upload')
+      .attach('file', Buffer.from('hello world'), 'test.txt');
+
+    expect(res.status).toBe(200);
+    expect(res.body.id).toBeDefined();
+
+    // Save this id so the download test can fetch the same file.
+    uploadedFileId = res.body.id;
+  });


These integration tests upload files to the server’s real uploads/ directory but never clean them up, so repeated local test runs will accumulate artifacts on disk. Consider using a temp uploads directory for NODE_ENV=test and/or deleting uploaded files in afterAll.

server/package.json

server/index.js

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

JaivPatel07 · 2026-03-21T17:49:46Z

I have updated all the required changes

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.

Files not reviewed (1)

server/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-21T18:22:18Z

server/test/index.test.js

+    // Save this id so the download test can fetch the same file.
+    uploadedFileId = res.body.id;
+  });
+
+  // Sending no file should return a validation error.
+  it('should fail if no file is sent', async () => {
+    const res = await request(baseURL).post('/upload');
+
+    expect(res.status).toBe(400);
+  });
+});
+
+// File download tests
+describe('Download File', () => {
+  // Downloads the file uploaded in the previous test block.
+  it('should download file using valid id', async () => {
+    const res = await request(baseURL).get(`/download/${uploadedFileId}`);
+


uploadedFileId is populated in one test and then consumed by a different describe block. This makes the suite order-dependent and can fail if tests are retried/shuffled or if the upload test fails (download becomes /download/undefined). Prefer creating the upload in a beforeAll within the download suite, or uploading within the download test itself.

server/test/index.test.js

server/index.js

server/test/index.test.js

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

JaivPatel07 · 2026-03-22T03:31:50Z

Done

Copilot

Pull request overview

Copilot reviewed 5 out of 6 changed files in this pull request and generated 8 comments.

Files not reviewed (1)

server/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T08:13:04Z

server/test/index.test.js

+  it('should connect and receive init message', async () => {
+    client1 = new WebSocket('ws://localhost:4000');
+
+    const msg = await waitForMessageOfType(client1, 'init');
+    expect(msg.id).toBeDefined();
+  });


client1/client2 are created without any 'error' handler. If the WS connection fails for any reason, ws will emit an 'error' event which (without a listener) can crash the test process. Consider reusing connectAndInitClient() for these cases or attaching an error handler in the test to fail fast/cleanly.

Copilot · 2026-03-24T08:13:05Z

server/test/index.test.js

+beforeAll(async () => {
+  const PORT = 4000;
+
+  await new Promise((resolve) => {
+    server.listen(PORT, () => {
+      baseURL = `http://localhost:${PORT}`;
+      resolve();
+    });
+  });


The test server is bound to a fixed port (4000) and the listen Promise never rejects on EADDRINUSE (it only resolves on the listening callback). This can make CI/local runs flaky or hang indefinitely if the port is taken. Prefer listening on an ephemeral port (server.listen(0)) and deriving baseURL/WS_URL from server.address().port, and add an 'error' handler that rejects the Promise.

Copilot · 2026-03-24T08:13:05Z

server/test/index.test.js

+afterAll(async () => {
+  terminateClient(client1);
+  terminateClient(client2);
+
+  await new Promise((resolve) => {
+    server.close(() => {
+      resolve();
+    });
+  });


afterAll always calls server.close(). If beforeAll failed to bind (or the server was never started), server.close() will throw ERR_SERVER_NOT_RUNNING and can mask the real failure. Consider guarding with server.listening (or tracking whether listen succeeded) and only closing when the server is actually listening.

Copilot · 2026-03-24T08:13:05Z

server/test/index.test.js

+    client2 = new WebSocket('ws://localhost:4000');
+
+    const msg = await waitForMessageOfType(client1, 'peer-joined');


This test attaches the 'peer-joined' listener after creating client2. If the server emits 'peer-joined' to client1 very quickly, the message can arrive before waitForMessageOfType() subscribes, causing a flaky failure. Safer pattern is to start waiting (set up the listener) before triggering the second connection.

Suggested change

client2 = new WebSocket('ws://localhost:4000');

const msg = await waitForMessageOfType(client1, 'peer-joined');

const peerJoinedPromise = waitForMessageOfType(client1, 'peer-joined');

client2 = new WebSocket('ws://localhost:4000');

const msg = await peerJoinedPromise;

Copilot · 2026-03-24T08:13:05Z

server/test/index.test.js

+describe('Single File Upload', () => {
+  // Upload one file and store the returned id for later download tests.
+  it('should upload file and return file id', async () => {
+    const res = await request(baseURL)
+      .post('/upload')
+      .attach('file', Buffer.from('hello world'), 'test.txt');
+
+    expect(res.status).toBe(200);
+    expect(res.body.id).toBeDefined();
+
+    // Save this id so the download test can fetch the same file.
+    uploadedFileId = res.body.id;
+  });
+
+  // Sending no file should return a validation error.
+  it('should fail if no file is sent', async () => {
+    const res = await request(baseURL).post('/upload');
+
+    expect(res.status).toBe(400);
+  });
+});
+
+// File download tests
+describe('Download File', () => {
+  // Downloads the file uploaded in the previous test block.
+  it('should download file using valid id', async () => {
+    const res = await request(baseURL).get(`/download/${uploadedFileId}`);
+
+    expect(res.status).toBe(200);
+  });
+
+  // Unknown ids should return 404.
+  it('should return 404 for wrong id', async () => {
+    const res = await request(baseURL).get('/download/wrong-id');
+
+    expect(res.status).toBe(404);
+  });
+});


These tests share state via uploadedFileId set in a prior it block. If the runner reorders tests or runs them concurrently in the future, the download test can become flaky (uploadedFileId undefined). Consider uploading in a beforeAll for the download suite, or performing the upload within the same test that asserts download behavior.

Copilot · 2026-03-24T08:13:06Z

README.md

+
+### Prerequisites
+
+- Node.js 16+ and npm installed


The new test instructions say Node.js 16+, but the added test toolchain (vitest@4.1.0) declares Node >=20 in the lockfile. Update the README prerequisites to match the minimum Node version actually required to run npm test successfully.

Suggested change

- Node.js 16+ and npm installed

- Node.js 20+ and npm installed

Copilot · 2026-03-24T08:13:06Z

README.md

+Or run the npm test script (watch mode):
+
+```bash
+cd server
+npm test
+```


README says "Or run the npm test script (watch mode)", but server/package.json defines npm test as vitest run (non-watch, single run). Adjust the wording (or the script) so the described behavior matches what actually runs.

Copilot · 2026-03-24T08:13:06Z

.github/workflows/test.yml

+
+    steps:
+      - name: Checkout Code
+        uses: actions/checkout@v3


GitHub Actions is using actions/checkout@v3. Consider bumping to actions/checkout@v4 to stay on the currently supported major version and receive the latest fixes.

Suggested change

uses: actions/checkout@v3

uses: actions/checkout@v4

Add test instructions to README, implement test scripts, and create t…

b151a42

…est suite

JaivPatel07 closed this Mar 20, 2026

Update test workflow to use vitest for running tests

8695b5f

JaivPatel07 reopened this Mar 20, 2026

JaivPatel07 closed this Mar 20, 2026

chore: add devDependencies for testing tools (supertest, vitest)

0dfed49

JaivPatel07 reopened this Mar 20, 2026

chore: update Node.js version to 22 in test workflow

b0fafc4

DhanushNehru requested a review from Copilot March 21, 2026 12:44

Copilot started reviewing on behalf of DhanushNehru March 21, 2026 12:44 View session

Copilot AI reviewed Mar 21, 2026

View reviewed changes

JaivPatel07 and others added 3 commits March 21, 2026 23:15

Update .github/workflows/test.yml

7b744df

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update server/index.js

acd5d47

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update server/package.json

511a609

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

DhanushNehru requested a review from Copilot March 21, 2026 18:19

Copilot started reviewing on behalf of DhanushNehru March 21, 2026 18:19 View session

Copilot AI reviewed Mar 21, 2026

View reviewed changes

JaivPatel07 and others added 3 commits March 22, 2026 08:58

Update server/test/index.test.js

e2624ac

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update server/test/index.test.js

c5327b8

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update server/index.js

c22d666

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

DhanushNehru requested a review from Copilot March 24, 2026 08:08

Copilot started reviewing on behalf of DhanushNehru March 24, 2026 08:09 View session

DhanushNehru approved these changes Mar 24, 2026

View reviewed changes

Copilot AI reviewed Mar 24, 2026

View reviewed changes

DhanushNehru merged commit d5695d4 into DhanushNehru:main Mar 24, 2026
5 checks passed

		client2 = new WebSocket('ws://localhost:4000');

		const msg = await waitForMessageOfType(client1, 'peer-joined');

	- Node.js 16+ and npm installed
	- Node.js 20+ and npm installed

Uh oh!

Conversation

JaivPatel07 commented Mar 20, 2026

Add tests, CI workflow, and improve README

What’s included

Testing

CI setup

README updates

Notes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JaivPatel07 commented Mar 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JaivPatel07 commented Mar 22, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants