rubocop / minitest-style-guide Goto Github PK

Following are the assertion for which guides are still remaining. I am planning to work on them today and Raise PRs for each. Please do let me know if there is any objection or concern regarding adding any of the matchers or the way I work. 😄

FYI, I haven't used few of the matchers at all. I would be trying them on sample application and then raise the PRs 🙏

• assert_operator/ refute_operator
• assert_output/ refute_output
• assert_predicate/ refute_predicate
• assert_raises/ refute_raises
• assert_respond_to/ refute_respond_to
• assert_same / refute_same
• assert_send/ refute_send #15 (comment)
• assert_silent / refute_silent
• assert_throws / refute_throws

cc/ @koic @bbatsov

Feel free to share here resources that might be useful for the guide. I found a couple:

• https://launchschool.com/blog/assert-yourself-an-introduction-to-minitest
• https://semaphoreci.com/community/tutorials/getting-started-with-minitest

To start off the Mocha section (#37) I would propose these as recommended configuration settings:

 Mocha.configure do |c|
   c.stubbing_method_on_nil = :prevent
   c.stubbing_non_existent_method = :prevent
   c.stubbing_non_public_method = :prevent
 end

The first two can prevent subtle bugs which may cause false passes.

The third encourages better design by avoiding tests having knowledge of private implementation details.

https://mocha.jamesmead.org/Mocha/Configuration.html

.rubocop.yml

---
require:
  - rubocop-minitest

AllCops:
  NewCops: enable

test/test_minitest_assert_in_delta.rb

# frozen_string_literal: true

require 'minitest/autorun'

# test
class AssertInDeltaTest < Minitest::Test
  def test_assert_in_delta_cop
    refute_equal(4.2, 420_001 / 100_000.0)
  end
end

$ ruby test/test_minitest_assert_in_delta.rb
=> pass

$ rubocop

Offenses:

test/test_minitest_assert_in_delta.rb:10:5: C: [Correctable] Minitest/RefuteInDelta: Prefer using refute_in_delta(4.2, 420_001 / 100_000.0).
    refute_equal(4.2, 420_001 / 100_000.0)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

1 file inspected, 1 offense detected, 1 offense autocorrectable

$ rubocop -a
=> autocorrected

$ ruby test/test_minitest_assert_in_delta.rb
=> fail

Fails because the default delta value for refute_in_delta is 0.001. As Rubocop can never know the user's intended delta value this cop is not safe for autocorrect.

Don't know whether you'd consider Minitest/RefuteInDelta unsafe also, as if you replace the assertion in the above example with assert_equal, autocorrect turns a failing test into one that passes..

I'd also suggest the suggestion text for both cops use the good code examples where an explicit delta value is provided.

Are there any good conventions for naming individual tests?

I'm used to rspec with describe "#method" and then a context "when blah blah" and then it "does the thing", which is a pretty easy kind of format to follow.

But I'm trying out minitest and I'm not sure how to do naming for def test_does_the_thing_when_blah

As illustrated in https://github.com/ignacio-chiazzo/ruby-minitest-analyzer

Minitest uses Ruby classes, if a Minitest class inherits from another class, it will also inherit its methods causing Minitest to run the parent's tests twice. In some cases, we want them to run them twice, but most of the time, we don't.

(cc @ignacio-chiazzo)

Is your feature request related to a problem? Please describe.

Something I've noticed in our repositories at work, is that we often end up with a mixture of the various checks inside a single test-suite :

• _()
• value()
• expect()

Combined with the various checks like assert, must, refute, should.
I know several of them are being deprecated by minitest itself however.

Describe the solution you'd like

I'm wondering whether we can write a Cop, that takes a specific configuration to say that all checks need to be done with one of the three lines above, and should be followed by the required syntax, being must, should or other plugin.

The following methods have a note that they are "These are meant for library writers, NOT for regular test authors."

• after_setup
• after_teardown
• before_setup
• before_teardown

https://ruby-doc.org/stdlib-3.1.2/libdoc/minitest/rdoc/Minitest/Test/LifecycleHooks.html

I suspect people may incorrectly use these because are similarly named to the before and after hooks in RSpec.

(I think this would be a helpful addition to rubocop-minitest).

I recently learned about make_my_diffs_pretty! option from this post by @tomdalling.

The official docs warn that this is slower than the regular inspect, but when all tests are passing there should be no impact, so overall I'd say it's worth enabling for most users.

Style guide should not be offended by assert_equal(true, actual)

Rationale.

We were looking for a rubocop that enforced filenaming for minitest files, something similar to this one available when using RSpec:
https://www.rubydoc.info/gems/rubocop-rspec/1.0/RuboCop/Cop/RSpec/FileName

There is no cop like this in the out-of-the-box minitest cop repo:
https://github.com/rubocop-hq/rubocop-minitest

The minitest cop repo specifies that the listed cops are based on the recommendations in this style guide. While it would be easy for us to write a custom cop in our project to enforce the desired naming, I wonder if a more general change might be wanted? Before submitting a PR: How do contributors feel about adding a file naming recommendation to the style guide and in turn creating a file name enforcement cop?

Developers should be encouraged to supply a failure message when making single assertions.

In these cases, the code might be readable:

assert page.has_content?("Widget added to cart")

…but the test failure message doesn't give any info other than making developers go back to the code to see the error:

Expected false to be truthy.

I believe improving the test output is a net positive, both in situations where developer have easy access to the code (e.g. while performing TDD) and in situations where the test output is the main artifact (e.g. in continuous integration environments). But I realise that there might be other opinions, for example that adding "Expected widget to be added to card" is too much overhead for developers.

I think the ideal case is that assert helpers are more intelligent and can generate more useful messages automatically. The only way I can see to achieve that is for people to write custom assert_* helpers, but eventually those will end up in an assert call which again should have a failure message.

So I think the style guide should recommend always passing a failure message, especially to bare assert.

If this is a good idea, then this could also be included as a Rubocop cop. I only see one other cop that focuses on assert calls, Minitest/AssertWithExpectedArgument, which is focused on calls with more than one argument, and so doesn't lead to any ambiguous situations with this proposal.

Thoughts?

I understand that minitest has its own framework for mocking and stubbing, but it seems very common to use Mocha insead. Should the styleguide cover this?

Imagine the following statement

assert(!expr)

I think this should be rewritten to refute(expr). For symmetry, refute(!expr) should be rewritten to assert(expr), although I don't expect that to happen very often. And of course, the optional message should be handled as well.

We see a few more specific examples of this in the style guide. The first example of it is assert(!actual.nil?). This could (I'm not saying should) be deduced by an elimination process, where you first apply the elimination of the negated assert (resulting in refute(actual.nil?)), then apply the rule to use assert_nil. This would also provide a solution for multiple negations (e.g. assert(!!!!!expr) can be reduced to refute(expr) by applying the elimination rule 5 times).

The rubocop-minitest gem is pretty inconsistent regarding this subject, some rules include a single negation, some don't. Providing a higher level construct to eliminate these negations would result in a global fix (thus providing consistency), while keeping the cops themselves as simple as possible.