Add test suite for all current config loading systems #301
Conversation
test/config/yaml.test.ts
Outdated
| return error['stdout'].toString(); | ||
| } | ||
| // Unknown error | ||
| throw error; |
There was a problem hiding this comment.
This helper is great! Looks like it resolves #253 :)
Will be very useful going forward and testing CLI arguments, etc.
This looks like using the same exec function in the new test files, do you think it could be a single reusable test helper? There's a test/helpers folder for this.
There was a problem hiding this comment.
Sure thing! Will do that ASAP 😁
There was a problem hiding this comment.
Done ! I've used a "dumb" way where you need to pass the cli path, this could be enhanced further where the execFactory crawls back up to find the cli itself, but for a first iteration, I opted to keeping it KISS. Let me know what you think !
There was a problem hiding this comment.
Resolving the path to cli.js from the helper seems more ergonomic and clean, so would be great if you could still add that. Happy to merge either way!
There was a problem hiding this comment.
See the other comment, I've added it in the least intrusive way, since doing it the proper way could have an impact on test speed (fs apis to check for file and read files, for every test that uses the exec util)
Co-authored-by: Lars Kappert <lars@webpro.nl>
test/cli-preprocessor.test.ts
Outdated
| console.error(`Error during execution of command: ${command}`); | ||
| } | ||
| }; | ||
| const exec = execFactory(cwd, '../../dist/cli.js'); |
There was a problem hiding this comment.
Would it be an idea to resolve ../dist/cli.js in the helper module using import.meta.url from there? That would mean an argument less here :)
There was a problem hiding this comment.
It can be done yes, but then if the files are in a different location (like the npm run test script), then there need to be extra logic
const thisFileUrl = fileURLToPath(import.meta.url);
const cliPath = resolve(
thisFileUrl,
// Needed if run in the /tmp (like in the npm test script) to find the correct dist folder
thisFileUrl.endsWith('tmp/test/helpers/execKnip.js') ? '..' : '.',
'../../../dist/cli.js'
);The safest way is to go back up to find the package.json with the name knip, this way we are sure we are finding the right directory and is less prone to error if the test are moved around for example.
I've added the simple way, and I'll let you decide if it's worth the effort to look for the package.json
There was a problem hiding this comment.
Ah yes, that /tmp dir I forgot about.
Maybe something like require.resolve('dist/cli.js) to make an absolute path that should be OK from anywhere?
There was a problem hiding this comment.
It should be indeed ! Thank you for pushing the change directly to the branch :)
There was a problem hiding this comment.
Hah, I was waiting for CI to pass 💚
Wanted to get it done 🏓 Let's merge it?
Many many thanks for the great improvements!
There was a problem hiding this comment.
Let's merge it !
My pleasure ! I will experiment further with knip and the new dynamic capabilities ! I'll post my experiment on discord ;)
|
🚀 This pull request is included in v2.34.0. See Release 2.34.0 for release notes. |
This PR introduce two things :
A test suite to validate all config provider system, in a e2e manner :
knipwith aknip.jsfile that expose an objectknipwith aknip.jsfile that expose an async functionknipwith aknip.jsonfileknip -c knip.mjswith aknip.mjsfile that expose an objectknip -c knip.mjswith aknip.mjsfile that expose an async functionknipwith a config into apackage.json#knipknipwith aknip.tsfile that expose an async functionknipwith aknip.tsfile that expose a functionknipwith aknip.tsfile that expose an objectknip -c knip.yamlwith aknip.yamlfileAnd enhance the current config loading mechanism to support js function & async function as a config export. I've added :
If you need anything modified, feel free to reach out ! And thanks for your work on Knip ❤️