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

Fix ignore issues #346

Merged
merged 3 commits into from Oct 23, 2019
Merged

Fix ignore issues #346

merged 3 commits into from Oct 23, 2019

Conversation

danmayer
Copy link
Owner

These two issues came up while configuring Coverband for a new app... The vendor one, in particular, makes it look like we are missing data.

The move from include to match, is to keep the filters in sync we were allowing files in that we would filter out during reporting... This can lead to a lot of extra work for storage and merging.

@danmayer danmayer requested a review from kbaum October 23, 2019 19:42
# then just filtering them out of the final report
test 'ignores uses regex same as reporter does' do
regex_file = Coverband.configuration.current_root + '/config/initializers/fake.rb'
assert_equal true, @coverband.send(:track_file?, regex_file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think if possible we should try and avoid calling private methods and instance vars from our tests. Can make refactoring difficult later.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I know there is some advice about not testing private methods, I think we discussed it in another previous PR. I disagree with that testing advice as it makes tests far more complicated and makes it harder to interpret results. I think we will have different styles on this level of testing, which is cool as any project that has more than one dev has some divergent patterns.

Being able to target a highly specific method where complexity resides has at least for me, shown good value vs the tradeoffs of having to refactor tests when they change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense. Apologize for bringing the same issue again. Sometimes I think needing to test a private method, might be a good reason to extract the complicated logic into a class of its own.

Copy link
Owner Author

Choose a reason for hiding this comment

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

ha no worries, this seems to be one of the few things we have different coding opinions on ;)

Copy link
Collaborator

@kbaum kbaum left a comment

Choose a reason for hiding this comment

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

Great catch!

@danmayer danmayer merged commit 8dd9a3c into master Oct 23, 2019
@danmayer danmayer deleted the fix_ignore_issues branch October 23, 2019 21:43
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.

None yet

2 participants