adding a github action to allow npm tests to run on PR

[josev814]

We should be testing if our tests pass before merging into main. To prevent issues like this in the future.

Implementing this will allow us to prevent breaking changes from making it into main if a test fails

While the current product is using Node 18, 20 is the current LTS, so added testing for both Node 18 and 20

Test Files 2 failed | 10 passed (12)
Tests 2 failed | 61 passed | 5 skipped (68)
Start at 06:41:44
Duration 39.33s (transform 3.31s, setup 1.64s, collect 12.03s, tests 10.13s, environment 6.65s, prepare 2.83s)

Summary by CodeRabbit

• Tests

  • Tests now execute automatically on pull requests targeting the main branch across Node.js versions 18.x and 20.x.
  • Test coverage reporting is now enabled and generated in multiple formats.

• Chores

  • Updated testing framework and coverage tool dependencies to latest compatible versions.

[coderabbitai]

📝 Walkthrough

Walkthrough

The changes establish a comprehensive test infrastructure by configuring Vitest testing framework with coverage reporting capabilities, adding a GitHub Actions workflow to execute tests on pull requests targeting the main branch with Node.js version matrix (18.x and 20.x), and updating test-related dependencies and configuration files.

Changes

Cohort / File(s)	Summary
CI/CD & Test Configuration .github/workflows/test-on-pr.yml, vitest.config.mts	Added GitHub Actions workflow for running tests on PRs with Node.js matrix strategy; updated Vitest config to enable forked process pool and coverage reporting with text, text-summary, and HTML reporters.
Test Dependencies package.json	Updated vitest from ^1.0.0 to ^1.6.1; added @vitest/coverage-v8 and @vitest/ui at ^1.6.1.
Test Setup & Cleanup src/setupTests.ts, src/pages/Courses/CourseAssignments.test.tsx	Removed @testing-library/jest-dom/vitest import; removed whitespace in test assertions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰✨ A workflow springs to life so bright,
Tests shall run both left and right,
Coverage charts in colors glow,
Twenty, eighteen—watch them flow!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly describes the main change: adding a GitHub Action workflow to run npm tests on pull requests, which is the primary objective of the PR.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

vitest.config.mts (1)

25-27: The HTML coverage report is currently discarded in CI.

.github/workflows/test-on-pr.yml runs --coverage, but nothing uploads the generated coverage/ directory. That means every matrix leg pays to generate the HTML report and then throws it away. Either upload coverage/ as an artifact in the workflow or keep html as a local-only reporter.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@vitest.config.mts` around lines 25 - 27, The Vitest coverage config includes
'html' in coverage.reporter (in vitest.config.mts) but CI never preserves the
generated coverage/ HTML, so either remove 'html' from the reporter array to
stop generating discarded files or modify the CI workflow to upload the
coverage/ directory as an artifact; update coverage.reporter to
['text','text-summary'] if you choose the former, or add an artifact-upload step
in the CI job that runs tests to persist the coverage/ output if you choose the
latter.

src/test/setup.ts (1)

1-2: Use the Vitest-specific jest-dom entrypoint here.

This file is wired through Vitest setupFiles, so the supported setup import is @testing-library/jest-dom/vitest. Keeping the generic entrypoint makes the setup drift from the documented Vitest integration and can leave matcher typings/setup behavior inconsistent. (github.com)

♻️ Proposed fix

-import '@testing-library/jest-dom';
+import '@testing-library/jest-dom/vitest';
 import { expect, vi, afterAll, beforeAll } from 'vitest';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/setup.ts` around lines 1 - 2, The test setup currently imports the
generic entrypoint '@testing-library/jest-dom' which is incorrect for Vitest;
update the import to the Vitest-specific entrypoint
'@testing-library/jest-dom/vitest' in the setup module (the same file that
imports expect, vi, beforeAll, afterAll) so Vitest matcher wiring and typings
are applied correctly while leaving the existing named imports (expect, vi,
beforeAll, afterAll) unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/test-on-pr.yml:
- Around line 28-29: Replace the workflow step that currently runs "npm install"
with "npm ci" in the CI job (the step labeled "Install dependencies") so the
action uses the lockfile for reproducible installs and better
caching/performance; update the run command to "npm ci" and keep the step name
unchanged.

In `@src/pages/Courses/CourseAssignments.test.tsx`:
- Around line 76-80: The test currently makes the table assertion vacuous by
guarding it with "if (table)"; remove that conditional and assert the table
exists unconditionally (e.g., use screen.getByRole('table') or
expect(table).toBeInTheDocument() without the if) so a missing Table rendered by
CourseAssignments will fail the test; ensure the test still uses the existing
mock data matching mockCourseId so the Table should be present.

---

Nitpick comments:
In `@src/test/setup.ts`:
- Around line 1-2: The test setup currently imports the generic entrypoint
'@testing-library/jest-dom' which is incorrect for Vitest; update the import to
the Vitest-specific entrypoint '@testing-library/jest-dom/vitest' in the setup
module (the same file that imports expect, vi, beforeAll, afterAll) so Vitest
matcher wiring and typings are applied correctly while leaving the existing
named imports (expect, vi, beforeAll, afterAll) unchanged.

In `@vitest.config.mts`:
- Around line 25-27: The Vitest coverage config includes 'html' in
coverage.reporter (in vitest.config.mts) but CI never preserves the generated
coverage/ HTML, so either remove 'html' from the reporter array to stop
generating discarded files or modify the CI workflow to upload the coverage/
directory as an artifact; update coverage.reporter to ['text','text-summary'] if
you choose the former, or add an artifact-upload step in the CI job that runs
tests to persist the coverage/ output if you choose the latter.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ba321e26-c486-4b2a-be4d-1729f1f7ef08

📥 Commits

Reviewing files that changed from the base of the PR and between 474e9f7 and 7114994.

⛔ Files ignored due to path filters (1)

• package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (8)

• .github/workflows/test-on-pr.yml
• package.json
• src/pages/Assignments/__tests__/AssignmentEditor.test.tsx
• src/pages/Assignments/__tests__/CreateTeams.test.tsx
• src/pages/Courses/CourseAssignments.test.tsx
• src/setupTests.ts
• src/test/setup.ts
• vitest.config.mts

💤 Files with no reviewable changes (1)

• src/setupTests.ts

[josev814]

Test Files 2 failed | 11 passed (13)
Tests 8 failed | 45 passed | 12 skipped (65)
Errors 8 errors
Start at 23:52:55
Duration 39.51s (transform 2.98s, setup 1.92s, collect 10.36s, tests 7.80s, environment 9.56s, prepare 2.73s)

[josev814]

@kgudipe is there anything that would prevent this pr from going in? I think it would be beneficial to have PRs have tests ran before merging them.