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.

Sign up for GitHub

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

Jump to bottom

Add -Werror flag to turn Ruby warnings into errors #995

Closed
wants to merge 1 commit into from
Closed

Add -Werror flag to turn Ruby warnings into errors #995

wants to merge 1 commit into from

Conversation

casperisfine
Copy link

Fix: #990

When working on gems that support multiple Ruby versions, it can be tricky to noticed you introduced a warning as you generally develop locally with one specific version and rely on CI to ensure it still works on other Ruby versions.

I tested this manually with:

require "minitest/autorun"

class FooTest < Minitest::Test
  def test_thing
    def some_method
    end
  end

  private
    def some_method
    end
end
$ ruby -Ilib /tmp/test.rb -Werror
Run options: -Werror --seed 4820

# Running:

W

Finished in 0.000317s, 3154.5710 runs/s, 0.0000 assertions/s.

  1) Warning:
FooTest#test_thing [/tmp/test.rb:11]:
/tmp/test.rb:11: warning: method redefined; discarding old some_method


1 runs, 0 assertions, 0 failures, 0 errors, 0 skips

Open questions:

  • I'm not too sure how to properly test this feature, It would need some sort of integration test with a dedicated process, but unless I missed it, there is no such style of test currently, so I'm a bit unsure how best to do it.
  • I'm not 100% sure how I should report warnings, should I count them as error, failure, or their own count?

@byroot
Add -Werror flag to turn Ruby warnings into errors
75a62af 
Fix: minitest#990

When working on gems that support multiple Ruby versions,
it can be tricky to noticed you introduced a warning as you
generally develop locally with one specific version and rely
on CI to ensure it still works on other Ruby versions.
@zenspider zenspider self-assigned this Apr 12, 2024
@zenspider
Copy link
Collaborator

Taking a poke at it... I have a few questions rolling around in my brain:

  1. Should this be a different failure type (subclass of Assertion)?
  2. Should this be a mixin so you can convert one test class at a time?
  3. Should this failure be a "failure" (or new) or error? I know we like to call this Werror because of C but I consider failures vs warnings to be very different priorities (eg, I always focus on errors first).

I suspect my answers are: 1) hard maybe, 2) probably both mixin and cmdline flag that forces the mixin into Minitest::Test? 3) I suspect failure or subtype, not actually error.

@zenspider
Copy link
Collaborator

That was a bit fragmented... other possibilities:

Activating (or including) this could install a teardown that simply asserts that no warnings were collected, or wraps run_one_method in refute_warnings or or or... lots of interesting possibilities!

Thanks for opening this. I'm gonna enjoy seeing how this plays out.

zenspider
zenspider reviewed Apr 17, 2024
View reviewed changes
Copy link
Collaborator

@zenspider zenspider left a comment

Choose a reason for hiding this comment

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

some q's

lib/minitest.rb
when "error"
options[:error_on_warning] = true
$VERBOSE = true
::Warning[:deprecated] if ::Warning.respond_to?(:[]=) # Ruby 2.7+
Copy link
Collaborator

Choose a reason for hiding this comment

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

did you mean?

Suggested change
::Warning[:deprecated] if ::Warning.respond_to?(:[]=) # Ruby 2.7+
::Warning[:deprecated] = true if ::Warning.respond_to?(:[]=) # Ruby 2.7+

Copy link
Author

Choose a reason for hiding this comment

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

Yes.

lib/minitest.rb
else
if ::Warning.respond_to?(:[]=) # Ruby 2.7+
::Warning[s.to_sym] = true
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unfamiliar enough with Warning that I don't understand this side of the top branch... I can see that you're setting that category to true, but I don't see it doing anything? Clue me?

Copy link
Author

Choose a reason for hiding this comment

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

Warnings that have a category (:deprecated, :performance, :experimental), are only emitted if Warning[category_name] = true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Part of my confusion was that minitest/test_task always runs -w by default so Warning[:deprecated] was always true.

@casperisfine
Copy link
Author

I got some high priority sutff that came out, I'll try to come back to this at some point but may take a tiny while, sorry!

@zenspider zenspider mentioned this pull request May 3, 2024
Closed
@zenspider
Copy link
Collaborator

I've got some changes in flight here: https://gist.github.com/zenspider/391536c12501eff15894ddd9de9c4198

basically, add Warnings to summary output, improve location information (was all internal:), fix/refactor tests to work with both normal and -Werror

@zenspider
Copy link
Collaborator

merged my modifications... release soon

@zenspider zenspider closed this May 15, 2024
@ParadoxV5
Copy link

merged my modifications... release soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zenspider zenspider zenspider left review comments

Assignees

@zenspider zenspider

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

-Werror mode? - Turn warnings into errors
4 participants
@casperisfine @zenspider @ParadoxV5 @byroot