feat(cli)!: refactor factory/CLI to be more testable #725
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #725 +/- ##
==========================================
+ Coverage 86.72% 87.11% +0.38%
==========================================
Files 56 58 +2
Lines 7089 7288 +199
Branches 662 659 -3
==========================================
+ Hits 6148 6349 +201
+ Misses 940 938 -2
Partials 1 1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
nice improvements! some questions and a couple nits
regarding the aggregate packages config: I'd rather stay within the bounds of yargs for cli and action inputs defined in the workflow for the GH-action side:
- cli:
--packagesflag for the cli: json encoded string (or array of json strings)- consider adding
yargs.config()to make cli users' lives easier supporting actual nested json in the json config file rather than requiring embedded json encoded strings in the json config file) - cli users may happen to choose to store their CLI config somewhere in their repo but that doesn't affect us
- consider adding
- action:
packagesinput key: array of objects defined directly in the workflow file
| export const factory = { | ||
| githubRelease, | ||
| releasePR, | ||
| releasePRClass, | ||
| run, | ||
| runCommand, | ||
| }; |
There was a problem hiding this comment.
I like this common factory. nifty how you collected them together like this for a single entry point.
There was a problem hiding this comment.
It's maybe note the most "TypeScripty" looking code, but it's an approach I learned from working on Node:
https://github.com/nodejs/node/blob/master/lib/fs.js#L2058
I've found it makes it really easy to mock the methods, vs., static methods.
There was a problem hiding this comment.
This looks good to me!
In comparison to #581, one of the things that it looks like the google-github-actions/release-please-action (and other potential Node API users) will still have to handle is the empty command flow (calling both the github-release and release-pr in succession). I saw this logic being shared as way to create a single source of truth for release-please behaviors, especially with the idea of introducing new triggers (ie. the Google Cloud Build example). It seems like that could maybe still be done in a follow-up patch in addition to the shared changelog options parsing?
@jbottigliero if it's okay with you, let's think about this in a follow up. One slight concern I have, is that creating a GitHub Release, vs., creating a Release PR, take enough divergent config options, that we might still want to do some validation in the CLI and action before calling a factory? |
refactor!: helpers in factory class renamed. refactor!: more options pulled to top level (some types changed in process) refactor!: GitHub release now uses "run" rather than "createRelease" to execute
Co-authored-by: Joel Dodge <joeldodge@google.com>
src/factory.ts
Outdated
| } from './github-release'; | ||
| import {getReleasers} from './releasers'; | ||
|
|
||
| export function runCommand( |
There was a problem hiding this comment.
Do you need these extra exports if they are included into the single factory export?
There was a problem hiding this comment.
Is this extra factory necessary? The only place this is called is in the argv parsing and at that point you already know which command you're trying to execute.
There was a problem hiding this comment.
I was thinking it might be nice in the action, but maybe it's overkill.
There was a problem hiding this comment.
@chingor13 actually, one reason I would like to keep this approach, was that it made it really nice to test the CLI:
let classToRun: GitHubRelease;
// This logic is used to capture the class that would
// be executed, allowing us to validate that the appropriate
// properties were assigned from the argument parser:
sandbox.replace(
factory,
'run',
(runnable): Promise<undefined> => {
classToRun = runnable as GitHubRelease;
return Promise.resolve(undefined);
}
);
parser.parse(
'github-release --repo-url=googleapis/release-please-cli --package-name=cli-package'
);Because I was able to mock just the logic that runs the GitHub Release or Release PR, without actually running it.
Taking inspiration from @jbottigliero's work on #581, this PR refactors our factory class, and the CLI entry-point to be more easily tested/shared:
factory.runCommandhelper that can be used as an entry-point to GitHub releases or Release PRs.bin/release-please.tsdoes not execute unless it's being run bynode, allowing it to be tested in isolation.sinon.future work:
feat: packageName parameter is now optional
refactor!: helpers in factory class renamed.
refactor!: more options pulled to top level (some types changed in process)
refactor!: GitHub release now uses "run" rather than "createRelease" to execute
refactor!: drop unused proxy-key parameter.
CC: @joeldodge79