fix(github-release): release name is packageName #757
Conversation
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #757 +/- ##
==========================================
+ Coverage 90.02% 90.22% +0.20%
==========================================
Files 57 57
Lines 7490 7470 -20
Branches 674 715 +41
==========================================
- Hits 6743 6740 -3
+ Misses 746 729 -17
Partials 1 1
Continue to review full report at Codecov.
|
| exports['GitHubRelease createRelease attempts to guess package name for release 1'] = { | ||
| "tag_name": "v1.0.3", | ||
| "body": "\n* entry", | ||
| "name": "@google-cloud/foo v1.0.3", |
There was a problem hiding this comment.
we can see from the old (and stale as of #720) snapshot that the request payload to /repos/releases used to include the full package name
| "tag_name": "@google-cloud/foo-v1.0.3", | ||
| "body": "\n* entry", | ||
| "name": "@google-cloud/foo @google-cloud/foo-v1.0.3", |
There was a problem hiding this comment.
this one confuses me a bit. I would have expected it to be
"name": "@google-cloud/foo foo-v1.0.3"
I'm guessing it's because that prior to #720 either the way the test was setup or the actual source code was not correctly removing the @scope/ from the tag_name. Not a big deal, just pointing out that we have evidence of behavior that suggests we were not stripping out @scope/ from tag_name prior to #720 ...
| .withArgs( | ||
| '@google-cloud/foo', | ||
| 'foo-v1.0.3', | ||
| 'abc123', | ||
| '\n* entry', | ||
| false | ||
| ) |
There was a problem hiding this comment.
A few times now this withArgs has caused me to scratch my head for a minute when the code path stops sending the exact args. The result is undefined. Reading the docs you'd think that not matching the args would fall through to the original implementation but that's not the case. I know this is a "stub" as opposed to a "mock" so it shouldn't really be in the business of verifying that it's used properly. However, the mis-matched withArgs behavior results in confusing failures: "undefined has no property html_url" so... what to folks think about adding a rejects or throwsArg(0) to any stub that we use withArgs on in order to make diagnosing test failures easier?
It's done once here on L288 above. I suggest we try to always add it (except in cases where we need the stub to reject calls mimicking a 403 or 404 from the GH api) Thoughts?
There was a problem hiding this comment.
I agree, it'd be nice if it threw on an unexpected input which we do similar to the get file stubs. We can grab the stub and have the fallthrough case reject, but it's more verbose to write in the test.
|
cc @chingor13 |
joeldodge79
force-pushed
the
github-release-name-vs-tag-name-bugfix
branch
from
610af0b
to
2837716
Compare
Forming the contents of a release was factored out of GitHubRelease in googleapis#720 The argument provide for the release name was accidentally swapped to be the packagePrefix (e.g. sans @scope for node packages).
joeldodge79
force-pushed
the
github-release-name-vs-tag-name-bugfix
branch
from
62ad6b0
to
2983cf4
Compare
bcoe
added
the
automerge
gcf-merge-on-green
bot
removed
the
automerge
Forming the contents of a release was factored out of GitHubRelease in
#720
The argument provide for the release name was accidentally swapped to be
the packagePrefix (e.g. sans
@scope/for node packages).