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

Check for assertion method receiver - fixes #321 #322

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

MatzFan
Copy link

@MatzFan MatzFan commented Sep 26, 2024

Counting assertions based on a string prefix of "assert_" or "refute_" is replaced by a symbol comparison against the 38 Minitest assertion methods.

@Earlopain
Copy link
Contributor

This is not the best solution. It will ignore all custom assertions the user may have written themselves, and test methods provided by gems like rails.

I think it would be better to instead only look at methods that have no receiver.

@MatzFan
Copy link
Author

MatzFan commented Sep 26, 2024

@Earlopain

It will ignore all custom assertions the user may have written themselves, and test methods provided by gems like rails.

Apologies, I didn't see that tested for anywhere.

I think it would be better to instead only look at methods that have no receiver.

Or where the receiver is not a kind_of Minitest::Test?

@Earlopain
Copy link
Contributor

Apologies, I didn't see that tested for anywhere.

I guess the assert_something call you changed could be counted as that but yes, it isn't very explicit in the test suite yet. Let's just add a testcase for custom assertion methods with this PR.

Or where the receiver is not a kind_of Minitest::Test?

Perhaps, but not decidable by rubocop. It can't tell if MyCustomTestCase or AcitveSupport::TestCase inherit from Minitest::Test. Just no receiver seems fine, unless you see a problem with that yourself.


Please add a changelog entry for this, and write a meaningful commit message starting with [Fix #321]. Thanks!

@MatzFan
Copy link
Author

MatzFan commented Sep 26, 2024

Will take a 👀

@MatzFan MatzFan changed the title fix #321 Check for assertion method receiver - fixes #321 Oct 6, 2024
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