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 Minitest assertion annotations #276

Merged
merged 1 commit into from
Aug 12, 2024
Merged

Conversation

vinistock
Copy link
Member

Type of Change

  • Add RBI for a new gem
  • Modify RBI for an existing gem
  • Other:

Changes

Now that we have untyped statistics, better typing Minitest assertions is important to remove them from the data - since they don't actually indicate any lack of safety. The reason is because we invoke so many assertions in the many many tests we have that they end up polluting the results with a bunch of untypeds.

This PR adds Minitest annotations, but only for assertions. Most of the other methods defined in the Minitest gem are only invoked by the gem itself, developers almost always interact only with assertions.

Note

I have tested this on Core and it found maaaany cases of the incorrect usage of custom failure messages:

# Correct
assert_equal(1, 2, "One is not two!!")

# Incorrect
assert_equal(1, 2, msg: "One is not two!!")
assert_equal(1, 2, message: "One is not two!!")
assert_equal(1, 2, { message: "One is not two!!" })

The fixes are quite straight forward, but it will be a big PR.

@vinistock vinistock self-assigned this Aug 8, 2024
@vinistock vinistock requested a review from a team as a code owner August 8, 2024 17:32
rbi/annotations/minitest.rbi Outdated Show resolved Hide resolved
@andyw8
Copy link
Contributor

andyw8 commented Aug 8, 2024

Worth having a type alias for T.nilable(T.any(String, Symbol, Proc))?

rbi/annotations/minitest.rbi Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs-add-minitest-assertions branch 3 times, most recently from 1263ada to 5b58dec Compare August 8, 2024 19:29
@vinistock
Copy link
Member Author

Worth having a type alias for T.nilable(T.any(String, Symbol, Proc))?

That's not possible because our CI checks to see if all constants exist in the runtime and the type alias wouldn't.

rbi/annotations/minitest.rbi Outdated Show resolved Hide resolved
rbi/annotations/minitest.rbi Outdated Show resolved Hide resolved
rbi/annotations/minitest.rbi Outdated Show resolved Hide resolved
rbi/annotations/minitest.rbi Outdated Show resolved Hide resolved
rbi/annotations/minitest.rbi Outdated Show resolved Hide resolved
@vinistock vinistock force-pushed the vs-add-minitest-assertions branch 2 times, most recently from 32fd09d to b1c0d93 Compare August 8, 2024 21:12
Copy link
Contributor

@Morriar Morriar left a comment

Choose a reason for hiding this comment

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

Can we sort these annotations by name?

@amomchilov
Copy link
Contributor

@Morriar Better yet, I'd rather have them in the same order as https://github.com/minitest/minitest/blob/master/lib/minitest/assertions.rb

Like so: 8cf2e73

@vinistock
Copy link
Member Author

Sorted based on how they are declared in Minitest and improved the signature for assert_raises, which now returns the right type depending on the exception classes passed.

@vinistock vinistock force-pushed the vs-add-minitest-assertions branch 2 times, most recently from 5df98d2 to 8c4df54 Compare August 12, 2024 17:07
@vinistock vinistock merged commit ea30c07 into main Aug 12, 2024
4 checks passed
@vinistock vinistock deleted the vs-add-minitest-assertions branch August 12, 2024 17:48
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.

4 participants