-
Notifications
You must be signed in to change notification settings - Fork 70
Modernize all test files to use vitest expect patterns #507
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
Conversation
- Replace assert with expect from vitest - Use path.join instead of string concatenation for paths - Update all assertions to use vitest patterns: - assert.strictEqual() → expect().toBe() - assert.deepStrictEqual() → expect().toEqual() - assert.throws() → expect().toThrow() - assert() → expect().toBeTruthy() 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…est expect - Replace assert with expect from vitest - Update all assertions to use vitest patterns: - assert.strictEqual() → expect().toBe() - assert.deepStrictEqual() → expect().toEqual() 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Replace assert with expect from vitest - Update all assertions to use vitest patterns: - assert.strictEqual() → expect().toBe() - assert.equal() → expect().toBe() - assert() → expect().toBeTruthy() - assert(!x) → expect(x).toBe(false) - Use toBeFalsy() for drive.disc check to better capture intent (original assert.equal(undefined, null) passed due to loose equality) - Add guidance in CLAUDE.md to use idiomatic vitest assertions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR modernizes all unit tests to use Vitest’s expect
API instead of Node’s assert
module, ensuring consistency with existing Vitest patterns and updating documentation accordingly.
- Updated test imports to include
expect
and removed the Nodeassert
import. - Replaced
assert.*
calls withexpect(...).toBe()
or.toEqual()
as appropriate. - Added a note in
CLAUDE.md
encouraging idiomatic Vitest assertions.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tests/unit/test-utils.js | Swapped assert calls for expect and adjusted import |
tests/unit/test-tokenise.js | Replaced assert with expect and updated import |
tests/unit/test-scheduler.js | Converted assert checks to expect calls |
tests/unit/test-intel-fdc.js | Changed assert.equal to expect().toBe() |
tests/unit/test-gzip.js | Updated file-path handling and switched to expect |
tests/unit/test-fifo.js | Swapped assert calls for expect |
tests/unit/test-disc-drive.js | Replaced assertions with expect , used expect.fail |
tests/unit/test-bcd.js | Updated assert.strictEqual to expect syntax |
CLAUDE.md | Added guidance to use Vitest expect assertions |
tests/unit/test-utils.js
Outdated
assert(arr instanceof Uint8Array); | ||
assert.strictEqual(arr.length, str.length); | ||
assert.strictEqual(uint8ArrayToString(arr), str); | ||
expect(arr instanceof Uint8Array).toBeTruthy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using the toBeInstanceOf
matcher for clarity: expect(arr).toBeInstanceOf(Uint8Array)
.
expect(arr instanceof Uint8Array).toBeTruthy(); | |
expect(arr).toBeInstanceOf(Uint8Array); |
Copilot uses AI. Check for mistakes.
assert.equal(drive.disc, null); | ||
assert(!drive.spinning); | ||
expect(drive.trackLength).toBe(IbmDiscFormat.bytesPerTrack); | ||
expect(drive.disc).toBeFalsy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since drive.disc
is expected to be null
, you can use the more precise matcher toBeNull()
: expect(drive.disc).toBeNull()
.
expect(drive.disc).toBeFalsy(); | |
expect(drive.disc).toBeNull(); |
Copilot uses AI. Check for mistakes.
tests/unit/test-tokenise.js
Outdated
@@ -21,9 +20,9 @@ describe("Tokeniser", function () { | |||
const t = await tokeniser; | |||
t.tokenise(text); | |||
console.log("Failed to give exception with message:", expectedError); | |||
assert.strictEqual(false, true); | |||
expect(false).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use expect.fail()
for unreachable branches instead of expect(false).toBe(true)
to improve test readability.
expect(false).toBe(true); | |
expect.fail("Unreachable code executed: expected an exception."); |
Copilot uses AI. Check for mistakes.
- Use toBeInstanceOf() for cleaner instanceof checks in test-utils.js - Use expect.fail() instead of expect(false).toBe(true) in test-tokenise.js - Keep toBeFalsy() for drive.disc check (not toBeNull) since actual value is undefined 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Summary
expect
assertions instead of node'sassert
moduleTest Files Updated
Key Changes
assert.strictEqual()
→expect().toBe()
assert.deepStrictEqual()
→expect().toEqual()
assert.throws()
→expect().toThrow()
assert()
→expect().toBe(true)
orexpect().toBeTruthy()
assert(\!expr)
→expect().toBe(false)
orexpect().toBeFalsy()
expect().toBeFalsy()
for null/undefined checks where appropriateTest Plan
🤖 Generated with Claude Code