Google-cloud-ruby: Minitest 6 Spec DSL?

[blowmage]

Minitest 6 is changing the spec DSL and requiring all expectations be wrapped.

# Minitest 5
foo.must_equal 42

# Minitest 6
_(foo).must_equal 42

Recent Minitest releases are now warning when the wrapper is not present, which is adding significantly to the test output, making it difficult to read. I see the following options:

1. Pin the dependency to block the most recent releases from being used
2. Update all expectations to use the wrapper
3. Convert all expectations to use assertions instead (assert_equal 42, foo)

[TheRoyalTnetennba]

I vote for 3. 1 just kinda delays 2 or 3, and personally I'm not thrilled about how 2 looks/communicates.

[quartzmo]

I am good with either 2 or 3. It will be great to fix this soon, because the DEPRECATED output is really verbose.

[quartzmo]

I think for consistency with the generated packages, we should go with 3.

[dazuma]

Overall I'd like to go with 3. I'm okay with doing 1 temporarily during the transition, as I imagine it's an enormous task to migrate all the tests (even if we're able to automate parts of it).

[blowmage]

personally I'm not thrilled about how 2 looks/communicates

The DSL change is meant to track the evolution in RSpec's DSL. While the actual method is named _, you can use the aliases value or expect to be more communicative:

# Vanilla Minitest
_(foo).must_equal 42

# I kinda prefer this myself
value(foo).must_equal 42

# RSpec-y
expect(foo).must_equal 42

[quartzmo]

@blowmage How do you feel about 3. ?

[blowmage]

How do you feel about 3. ?

I have a love/hate with the spec DSL, but I definitely love consistency. I would prefer either using the spec DSL or not, and 3. feels like a half measure. But my opinion is weakly held, and I’m fine with whatever is decided.

[blowmage]

We have already implemented option 1. I think this is a temporary measure, and I think we should also discuss the long term solution. I therefore propose the following options:

• 🎉 Full Spec DSL: With this option we will keep both the spec DSL's describe/it block structure, as well as the expectations by adding expect/value/_ wrapper calls before a must_ or wont_ call.
• ❤️ Hybrid Spec DSL: With this option we will keep the spec DSL's describe/it block structure, but use assertions instead of expectations. That means replacing all must_ or wont_ calls with assert_ or refute_ methods.
• 🚀 Drop Spec DSL: With this option we will replace both the spec DSL's describe/it block structure, as well as the expect_ or refute_ calls. They will be replaced with test class definitions using test_methods, and assert_ or refute_ methods.

@googleapis/yoshi-ruby Can you vote for your preferred direction using the emoji buttons?

[yahonda]

Hi, I'm interested in google-cloud-storage Ruby 3 keyword arguments support. It would also need to bump/unlock the minitest version to 5.12 or higher.

Thanks to rubocop-minitest gem, it could address most of the deprecation warnings of global expectations. Let me open a pull request.

[quartzmo]

Per the survey taken above in the comment from Nov 11, 2019, the decision was to:

keep the spec DSL's describe/it block structure, but use assertions instead of expectations. That means replacing all must_ or wont_ calls with assert_ or refute_ methods.

@dazuma @TheRoyalTnetennba Do you still feel the same today?

[quartzmo]

@yahonda has very generously provided #5207, but it uses the _ wrapper rather than converting to assert_ methods. (The _ wrapper offers a much easier conversion, to be sure.)

[quartzmo]

Given the large number of usages to update across google-cloud-ruby, I am changing my vote to:

🎉 Full Spec DSL: With this option we will keep both the spec DSL's describe/it block structure, as well as the expectations by adding expect/value/_ wrapper calls before a must_ or wont_ call.

Meaning that I am OK with the updates in #5207.

[dazuma]

I don't mind it in the interim, since it's a largely mechanical conversion, and we're probably not realistically going to be converting to asserts any time soon.

[quartzmo]

Is the change to the _ wrapper in #5207 OK with you @TheRoyalTnetennba? Likely to be a sticky decision, I'm guessing.

[TheRoyalTnetennba]

I think with both the roll out of the microgenerator, and the migration of the samples to minitest, we should just pick the one we like best and stick with it. This is (probably) the least worst time to change. I tend to value consistency.

Happy to go with consensus. I kinda just like ❤️ Hybrid Spec DSL better. But whatever we go with we should do for microgenerator and samples too.

[quartzmo]

Existing styles:

• veneer
• micro-generator
• samples

[quartzmo]

@yahonda How do you feel about converting your PR to the "❤️ Hybrid Spec DSL"? See the samples example. Basically, this means replacing all must_ or wont_ calls with assert_ or refute_ assertions.

[dazuma]

Sorry, I'm going to override the above suggestion @yahonda. Let's leave your PR as-is, because it works as-is, and satisfies the immediate need for Ruby 2.7/2.8. Yes, we do have longer-term aspirations to standardize on assertions, but that would be a lot more effort, and it can be put off to later.

[quartzmo]

I agree. @yahonda: Let's leave your PR as-is, because it works as-is. Thank you! I'll look through the PR soon for other issues.

[yahonda]

Thanks for the discussion. Sure, let me leave #5207 as-is.