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

Add status filtering to GitHub events #1669

Merged
merged 5 commits into from Dec 29, 2017

Conversation

joshsmith
Copy link
Contributor

@joshsmith joshsmith commented Dec 29, 2017

WIP.

@joshsmith
Copy link
Contributor Author

I would really rather not add a ton of acceptance tests here. The downside of the added test run time is not really worth any guarantees seen by very few users.

Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

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

Looks good. I generally agree with keeping acceptance tests to a minimum.

However, I also strongly believe we should test all features of the app with an acceptance test.

It may seem the two are at odds, but really, for me, that means we write big tests. If a single test can cover the whole feature sufficiently, such as the case of this PR, that's perfect.

The bulk of the slowness of an acceptance test is the app startup anyway, as far as I Know.

Generally, integration and unit tests should be small, but acceptance tests make sense to be big.

@begedin
Copy link
Contributor

begedin commented Dec 29, 2017

Added the missing assertions for the clear button

@joshsmith
Copy link
Contributor Author

I don’t think we’re terribly disagreed on acceptance tests, so curious what you think of this post: https://dockyard.com/blog/2016/03/28/prefer-integration-tests

@begedin begedin merged commit 8a2130d into develop Dec 29, 2017
@begedin begedin deleted the add-status-filtering-to-github-events branch December 29, 2017 17:44
joshsmith added a commit that referenced this pull request Dec 29, 2017
Add status filtering to GitHub events (#1669)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants