-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fixes #324 Use branch name from config when creating PR #353
Fixes #324 Use branch name from config when creating PR #353
Conversation
Another solution would be to have the default value in |
I tried to write a test for this, but just
which seemed very weird |
Even weirder:
does work! |
Added a test (which mocks the GitHub API - #330). |
Updated snapshots |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good - just seems to have some changes that are unrelated or unintentional. I think we should squash "Add test" into the main commit and remove the snapshot update from the PR, in addition to my comments on specific pieces.
package.json
Outdated
@@ -68,7 +68,7 @@ | |||
"file-loader": "^0.8.5", | |||
"identity-obj-proxy": "^3.0.0", | |||
"imports-loader": "^0.6.5", | |||
"jest-cli": "^16.0.1", | |||
"jest-cli": "19.1.0-alpha.eed82034", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we updating this and jest
below to a specific alpha version? This looks like it was an unintentional change - if so, can you remove this from the commit?
EDIT: saw your reasoning for adding this above. IDK if I'm comfortable with pinning our test tooling to a specific alpha version - @erquhart what are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as long as the tests pass, it's fine. But we should update it as soon as there is a stable version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd second that it's fine, mostly because it's a dev dependency. Let's keep an eye out to update soon.
yarn.lock
Outdated
@@ -59,11 +59,11 @@ JSONStream@^0.8.4: | |||
jsonparse "0.0.5" | |||
through ">=2.2.7 <3" | |||
|
|||
abab@^1.0.0: | |||
abab@^1.0.3: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these changes coming from the updated jest
and jest-cli
dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the abab
dependency comes from jsdom
which is used by jest
.
@@ -0,0 +1,39 @@ | |||
import AssetProxy from "../../../valueObjects/AssetProxy"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic - mocking the GH API is an existing issue (#330). This is a great step towards that, which will let us add a test suite for the editorial workflow.
@@ -1,3 +1,5 @@ | |||
exports[`EntryEditorToolbar should disable and update label of Save button when persisting 1`] = `"<div><button disabled=\"\" class=\"\" type=\"button\" data-react-toolbox=\"button\">Saving...</button> <button class=\"\" type=\"button\" data-react-toolbox=\"button\">Cancel</button></div>"`; | |||
// Jest Snapshot v1, https://goo.gl/fbAQLP |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These snapshot changes shouldn't be a part of this PR - it looks like these are due to either the jest
update above or a previous commit not updating the snapshots when they should have been. Either way, this is unrelated to the rest of the PR, and should probably be handled separately.
@Benaiah the jest updates were because '.resolve' for testing promises was only introduced to jest recently (and will be released in 20.0.0). The snapshot updates were because jest 19+ has a new escaping syntax for snapshots. |
I'll see if there's an easy way of testing promises with an earlier version of jest |
@josephearl I see. As far as I can tell, though, that version notation will keep us on 19.1.0-alpha even after 20 is released - is that the case? |
@Benaiah I see your point, yes I believe it will, let me fix that |
a5a3f56
to
3aaafaf
Compare
@Benaiah I've rebased and updated the |
@Benaiah let me if that's OK - or if we should look at keeping on a release version of jest and using another method to test the promise. |
@@ -386,7 +386,7 @@ export default class API { | |||
return this.deleteRef("heads", branchName); | |||
} | |||
|
|||
createPR(title, head, base = "master") { | |||
createPR(title, head, base = this.branch) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wondering if that Super expression
error you saw was related to this use of this
. The fact that we didn't get to the bottom of that error, and that the preceding import mysteriously stopped it, gives me pause. We should dig a little deeper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested -- I still get the error if I change it back to base = "master"
and use this.createPR(options.commitMessage, branchName, this.branch)
, so it seems not
@josephearl the consensus is that it's fine - we'll just want to update to a stable version as soon as it's available. |
3aaafaf
to
213d645
Compare
@Benaiah 👍 rebased |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@josephearl Jest has released v19.0.2 - could you update this PR to use the non-alpha version? Once that's complete this should be good to go. |
@Benaiah this is using the alpha of v19.2.0, not v19.0.2 |
@josephearl oh, my bad. I'll give it one more look over and merge then. Thanks for the contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
conflict on yarn.lock 😭 |
213d645
to
f3643a5
Compare
@calavera conflict resolved 😄 |
Awesome! Thanks 🎉 |
- Summary
Fixes #324 - Publishing using editorial workflow does not merge with correct branch when using Github backend
- Test plan
Tested correct branch name from config is passed to
createPR
as the base branch.- Description for the changelog
Pass the branch name from the config to
createPR
as the base branch.