Skip to content

Improve tests following lint advice from eslint-node-test - #2560

Merged
shadowspawn merged 19 commits into
tj:developfrom
shadowspawn:feature/eslint-node-test
Jul 27, 2026
Merged

Improve tests following lint advice from eslint-node-test#2560
shadowspawn merged 19 commits into
tj:developfrom
shadowspawn:feature/eslint-node-test

Conversation

@shadowspawn

@shadowspawn shadowspawn commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Problem

I transferred a lot of tests to a new testing framework, which I learnt as I went. There is some room for improvement. 😊

Solution

Install https://github.com/sindresorhus/eslint-node-test and fix the lint errors.

Some of changes:

  • two tests were duplicated
  • LOTS of (sensible) errors to check error when using assert.throws, as otherwise even a syntax error would pass
  • have just a single line of code in assert.throws()
  • consistently use assert.ok(expression) instead of assert(expression)
  • tidy some test names

ChangeLog


const program = new MyCommand();

program.configureHelp({ MyCommand });

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a bogus call I spotted myself, not one reported by the linter.

@shadowspawn shadowspawn changed the title Feature/eslint node test WIP: Improve tests Jul 24, 2026
@shadowspawn

Copy link
Copy Markdown
Collaborator Author

I tried turning on all the rules to see what else looked good.

require-top-level-describe enforces the pattern I followed with a file-level describe for any files. It picked up a few cases where I had added tests after the describe block. For single test files, I manually disabled the check as nothing extra to add for context.

require-hook picked up the pattern I used for wrapping fixture loops inside a describe means the code is running at file load time rather than test time. It will be a noisy refactor, so I'll turn that on separately.

program.parse(['one', 'two'], { from: 'user' });
assert.deepEqual(passedArg, ['one', 'two']);
});

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Duplicate of above test

// semver minor versions. For now, also testing the error.message and that output occurred
// to detect accidental changes in behaviour.

function expectCommanderError(err, exitCode, code, message) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed expectCommanderError and put tests inline for clarity within tests.

});

test('when specify unknown command then throw CommanderError', (t) => {
const stderrSpy = t.mock.method(process.stderr, 'write', () => {});

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed mocks to suppress/check output and used createTestCommand() for clean output instead

assert.deepEqual(program.opts().comma, ['CCC']);
});

test('when option has default array then specified value is used instead of default (not appended)', () => {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Duplicate of above test

@shadowspawn shadowspawn changed the title WIP: Improve tests Improve tests following lint advice from eslint-node-test Jul 25, 2026
@shadowspawn

Copy link
Copy Markdown
Collaborator Author

Finished work, ready for review. No hurry.

(I had intended to open as draft, but missed that sorry. Did at least put WIP in title initially!)

@abetomo abetomo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

👍

@shadowspawn
shadowspawn merged commit 234b145 into tj:develop Jul 27, 2026
11 checks passed
@shadowspawn
shadowspawn deleted the feature/eslint-node-test branch July 27, 2026 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants