rubocop / minitest-style-guide

Best practices for writing your tests
https://minitest.rubystyle.guide
70 stars 15 forks source link

Feature Request : Enforce consistency #25

Closed coding-red-panda closed 4 years ago

coding-red-panda commented 5 years ago

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 :

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.

koic commented 5 years ago

I still don't get the point. Could you show a code examples of these three using Minitest?

coding-red-panda commented 5 years ago

Sure,

as I wrote above, there are three ways to perform checks inside minitest:


def test_if_equal
   _(1 + 1).must_equal(2)
end

def test_if_equal
   value(1+1).must_equal(2)
end

def test_if_equal
  expect(1+1).must_equal(2)
end

You can freely mix these variants inside a test suite. My recommendation is to create a Cop that takes a configuration parameter:

MiniTest/Consistency:
  Enabled: true
  Style: :expect

Which would flag example 1 and 2 as warning, with the error message that the expected form is expect() and not _() or value()

koic commented 5 years ago

Thank you for the explanation. I got an understanding of the proposal and concept. https://github.com/seattlerb/minitest/blob/ecb1afeb36f831d20f1f0f9081bd1b8e32ddbdfe/lib/minitest/spec.rb#L291-L320

The cop will make the style consistent as suggested. OTOH, the cop name Consistency is abstract, so a more specific name is likely to be preferred :-)

coding-red-panda commented 5 years ago

sure all for better naming :) I'm just horrible at it like any other developer :D But the link is exactly what I was talking about.

koic commented 5 years ago

Ah, I forgot the important point. This issue should first be opened in The Minitest Style guide. So, I will transfer this issue to The Minitest Style Guide repo.

coding-red-panda commented 5 years ago

Should I make a PR for the Style guide once we have a good name for the cop ? Or just add the general idea?

koic commented 5 years ago

Here are some points I'm interested in adding to the rules in The Minitest Style guide.

coding-red-panda commented 5 years ago

I don't think there's a problem with choosing/using any of the particular methods themselves. If I read the code correctly, the core method is _ and the other two are just an alias for the method. Minitest however is encouraging/forcing people to use these methods instead of the global object methods that it originally implemented. (see the deprecation warnings), so in short, I don't think there's a problem with limiting/forcing a certain style, outside gaining the consistency in the code.

As for the second point...my personal preference would be to use expect to be inline with the RSpec DSL and most other testing frameworks from different languages.

metaskills commented 5 years ago

Which method is preferred for The Minitest Style Guide

Agreed with @coding-bunny on the second point too, I would prefer expect since it aligns with other testing frameworks.

duduribeiro commented 5 years ago

I don't get this one 🤔

the minitest way to check if values are equal is:

def test_if_equal
   assert_equal 2, something
end
tejasbubane commented 4 years ago

@duduribeiro This is the minitest spec syntax: http://docs.seattlerb.org/minitest/Minitest/Expectations.html

tejasbubane commented 4 years ago

Which method is preferred for The Minitest Style Guide If one method is consistently OK?

@koic Since the minitest docs are consistent with _, I think that is what we should suggest. Of course the cop will have configuration option to choose others.

coding-red-panda commented 4 years ago

@tejasbubane agreed with that. Documentation examples as default and the rest configured. And then keep the code consistent.

Bonus if we can support auto correct.

koic commented 4 years ago

@tejasbubane Sounds good to me. I quoted from Minitest documentation.

it "should still work in threads" do
  my_threaded_thingy do
    (1+1).must_equal 2                  # bad
    assert_equal 2, 1+1                 # good
    _(1 + 1).must_equal 2               # good
    value(1 + 1).must_equal 2           # good, also #expect
    _ { 1 + "1" }.must_raise TypeError  # good
  end
end

http://docs.seattlerb.org/minitest/Minitest/Expectations.html

I think that the style would be:

The Minitest Style Guide

A part of this will be solved by https://github.com/rubocop-hq/rubocop-minitest/pull/63.

# bad
(1+1).must_equal 2

# good
_(1 + 1).must_equal 2
value(1 + 1).must_equal 2
expect(1 + 1).must_equal 2

Minitest Cop

This will be implemented as a different Minitest cop than https://github.com/rubocop-hq/rubocop-minitest/pull/63.

# default
_(1 + 1).must_equal 2      # good
value(1 + 1).must_equal 2  # bad
expect(1 + 1).must_equal 2 # bad

# optional (value)
_(1 + 1).must_equal 2      # bad
value(1 + 1).must_equal 2  # good
expect(1 + 1).must_equal 2 # bad

# optional (expect)
_(1 + 1).must_equal 2      # bad
value(1 + 1).must_equal 2  # bad
expect(1 + 1).must_equal 2 # good

I'm not sure if implemetation of RuboCop Minitest should be enabled by default, anyway it could be included in the RuboCop Minitest.