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.

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

feat(backend): e2e tests for creating a pull request (#61) #62

Merged
merged 12 commits into from
May 7, 2020

Conversation

ermshiperete
Copy link
Collaborator

@ermshiperete ermshiperete commented May 4, 2020

This change is Reviewable

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

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

A few questions...

) {
throw new Error('Non-linear history. Force-push is not allowed.');
if (keyboardsHead !== keyboardsNoteInfo.commitSha && keyboardsNoteInfo.commitSha !== '') {
throw new HttpException('Keyboards repo has new changes. This is not allowed.', 409);
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the circumstances of this? The keyboards repo will continually get new changes so we need to handle that

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What I meant here is that the single-keyboard directory (that corresponds to the single-keyboard repo) has new changes.

if (keyboardsHead !== keyboardsNoteInfo.commitSha && keyboardsNoteInfo.commitSha !== '') {
throw new Error('Keyboards repo has new changes. This is not allowed.');
if (this.hasNonLinearHistory(singleKbNoteInfo, keyboardsNoteInfo)) {
throw new HttpException('Non-linear history in single-keyboard repo. Force-push is not allowed.', 400);
Copy link
Member

Choose a reason for hiding this comment

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

Is this just for a single PR? We don't allow force push for PRs generated through KDO?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, that's for a single PR and is relevant while a PR is open.

I wrote up #63 with some additional scenarios that the current implementation might not properly handle yet.

@mcdurdin
Copy link
Member

mcdurdin commented May 6, 2020

I note a failing test in the log...

This change makes calling git cross-platform compatible. Suppressing
stderr output can be done directly in node. This allows to run
the command on all platforms.
Now that I found the real reason for the "command not found" error
(there's no /dev/null on Windows), we can remove the workaround for
TC.
@ermshiperete ermshiperete merged commit e6617ce into master May 7, 2020
@ermshiperete ermshiperete deleted the feat/pull-request branch May 7, 2020 08:05
@ermshiperete ermshiperete added this to the P10S6 milestone May 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants