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

skip draft pull requests #615

Merged
merged 1 commit into from
Apr 3, 2019
Merged

skip draft pull requests #615

merged 1 commit into from
Apr 3, 2019

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Apr 2, 2019

github doesn't run any checks on draft PRs
emulate this behaviour by skipping prs as long as they are draft

fix: #596

github doesn't run any checks on draft PRs
emulate this behaviour by skipping prs as long as they are draft

fix: #596

Signed-off-by: Maxim Sukharev <[email protected]>
@smacker smacker requested review from carlosms and se7entyse7en April 2, 2019 15:56
@smacker
Copy link
Contributor Author

smacker commented Apr 2, 2019

re-pushed with the test

@se7entyse7en
Copy link
Contributor

LGTM 👍

if called {
cancel()
}
fmt.Fprint(w, `[{"id":1},{"id":2,"draft":true},{"id":3}]`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Wound't it be better to use lookout-test-fixtures here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. It's a unit test, usually, it's a bad idea to use huge fixtures in unit tests. Unit tests should test the smallest part of the logic possible and it should be easy to understand what they do.
  2. We don't have fixtures for PRs list in lookout-test-fixtures repository.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't entirely agree. In my opinion using fixture data does not automatically transform a unit test into an integration test. In this case the fixture is even vendored, so it's not so different from a hardcoded string.

But I don't want to block this PR for this.

@smacker smacker merged commit 2152962 into src-d:master Apr 3, 2019
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.

test how lookout behaves with draft PRs on github
3 participants