Skip to content
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

feat: Add error handling for nonexistent file case with --file option #5086

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions lib/cli/collect-files.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const minimatch = require('minimatch');
const {NO_FILES_MATCH_PATTERN} = require('../errors').constants;
const lookupFiles = require('./lookup-files');
const {castArray} = require('../utils');
const symbols = require('log-symbols');

/**
* Exports a function that collects test files from CLI parameters.
Expand Down Expand Up @@ -52,6 +53,22 @@ module.exports = ({
}
}, []);

// check that each file passed in to --file exists
fileArgs.forEach(file => {
const fileAbsolutePath = path.resolve(file);
try {
require.resolve(fileAbsolutePath);
} catch (err) {
// this error doesn't bubble up to the middleware like how
// --require handles it so we log a warning here and exit
const warningMessage = ansi.yellow(
`${symbols.warning} Warning: Cannot find test file "${file}" at the path "${fileAbsolutePath}"`
);
console.warn(`${warningMessage}\n`);
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

[Refactor] I don't love having a process.exit deep within code. n/no-process-exit has good reasoning around it.

It's also not really a "warning" yellow if the whole thing exits. At that point it's an error (red).

I think a more future-facing, approach would be either:

  • (less work, but still partially flawed) have this function throw a special error that any higher-up handler(s) know how to log nicely
  • (more work, but more understandable long-term) have this function return an object explaining its results, and have higher-up handler(s) work nicely with it

I haven't looked deeply enough to know whether either or both is feasible. Before we go into a whole deep dive, what do you think about this direction?

Copy link
Author

Choose a reason for hiding this comment

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

Good to learn about the no-process-exit, thanks! I think the latter where we return an object and having the higher-up handler(s) deal with it sounds like the approach here. The error wasn't handled in run-helpers so maybe we can add something so that bubbles up to run.js like the other methods?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that approach sounds like a good one to try!

}
});

// ensure we don't sort the stuff from fileArgs; order is important!
if (sort) {
specFiles.sort();
Expand Down
28 changes: 25 additions & 3 deletions test/integration/options/file.spec.js
JoshuaKGoldberg marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
'use strict';

var path = require('path').posix;
var helpers = require('../helpers');
var runMochaJSON = helpers.runMochaJSON;
var resolvePath = helpers.resolveFixturePath;
const {
runMochaJSON,
resolveFixturePath: resolvePath,
runMocha
} = require('../helpers');

describe('--file', function () {
var args = [];
Expand Down Expand Up @@ -64,4 +66,24 @@ describe('--file', function () {
done();
});
});

it('should log a warning if a nonexistent file is specified', function (done) {
Copy link
Member

Choose a reason for hiding this comment

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

[Naming]

Suggested change
it('should log a warning if a nonexistent file is specified', function (done) {
it('should log a warning if a nonexistent file with an unknown extension is specified', function (done) {

const nonexistentTestFileArg = 'nonexistent.test.ts';
runMocha(
nonexistentTestFileArg,
['--file'],
function (err, res) {
if (err) {
return done(err);
}

expect(res.output, 'to contain', 'Warning: Cannot find test file').and(
'to contain',
nonexistentTestFileArg
);
done();
},
{stdio: 'pipe'}
);
});
});