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.

Sign up for GitHub

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

Jump to bottom

feat: understands --config <FILE> option in conventionalChangelogArgs #927

Merged
merged 3 commits into from
Sep 25, 2020
Merged

feat: understands --config <FILE> option in conventionalChangelogArgs #927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2c57b65
feat: understands `--config <FILE>` option in `conventionalChangelogA…
aseure Sep 23, 2020
8f10fdd
Update packages/shipjs/src/step/prepare/__tests__/updateChangelog.spe…
Sep 25, 2020
15e03fd
fix test case
Sep 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions packages/shipjs/src/step/prepare/__tests__/updateChangelog.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
const path = require('path');
import { prepareParams } from '../updateChangelog';
import { parseArgs } from '../../../util';
import tempWrite from 'temp-write';

describe('prepareParams', () => {
it('loads configuration from --config option', () => {
parseArgs.mockImplementation(jest.requireActual('../../../util').parseArgs);
Copy link
Contributor

Choose a reason for hiding this comment

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

I had to restore the original implementation of parseArgs because I made a wrong choice in the beginning of Ship.js to globally mock many things including util. It's so hidden and hard to find out things like this :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes something was wrong with the parseArgs but I just discovered this morning that you finished the PR.

Just to understand: what does this test mock exactly? It seems we are mocking parseArgs but at the end, the config seems correctly generated, how does that work?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually parseArgs was not supposed to be mocked, but I mocked everything under util globally, which made parseArgs returned nothing. So it made the test case failed. This line parseArgs.mockImplementation(jest.requireActual('../../../util').parseArgs); is restoring its original implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, that's why it was returning me undefined and I couldn't see the console.log() I've added in its implementation! Thanks for letting me know. (my 2 cents on that, you should probably do it the other way around when testing: avoid to use global mocking, and use it explicitely when you need it in your tests).

const config = {
writerOpts: {
headerPartial: '## {{version}}',
},
};
const configString = `module.exports = ${JSON.stringify(config)};`;
const configPath = tempWrite.sync(configString);
const configDir = path.basename(path.dirname(configPath));

const { args } = prepareParams({
dir: configDir,
conventionalChangelogArgs: `-i CHANGELOG.md -s --config ${configPath}`,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed you added the -i/--infile option so that there is not the undefined error down the line with this flag. Too late for this PR as you already closed it, but maybe things could be improved here: arguments parsing shouldn't yield an error if a flag is missing, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right.

shipjs/packages/shipjs/src/step/prepare/updateChangelog.js

Lines 100 to 101 in 06c7226

args.infile = path.resolve(dir, args.infile);
args.outfile = path.resolve(dir, args.outfile);

We have these two lines expecting to have infile and outfile inside the parsed args. So the correct way to fix it would be to throw a better error message to dev and to document that these are mandatory.

revisionRange: '1.0.0..1.0.1',
reject: () => {},
});
expect(args.config).toEqual(config);
});
});
5 changes: 4 additions & 1 deletion packages/shipjs/src/step/prepare/updateChangelog.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ const argSpec = {
'-t': '--tag-prefix',
};

function prepareParams({
export function prepareParams({
dir,
conventionalChangelogArgs,
releaseCount,
Expand Down Expand Up @@ -118,6 +118,9 @@ function prepareParams({
}
const templateContext =
args.context && require(path.resolve(dir, args.context));
if (args.config) {
args.config = require(path.resolve(dir, args.config));
}
return { args, gitRawCommitsOpts, templateContext };
}

Expand Down