Skip to content

Introduce config.deprecation_warnings #161

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

byroot
Copy link
Contributor

@byroot byroot commented Dec 17, 2024

Fix: #37

Since Ruby 2.7.2, Ruby expect test frameworks to enable deprecation warnings so users get advance notice of future breaking changes.

I implemented it as specified in #37 (comment)

@byroot
Copy link
Contributor Author

byroot commented Dec 17, 2024

Hum, CI fails because of the 100% coverage target, but I don't know how I can satisfy this given some code path are only ever entered by some specific Ruby versions.

I'll try to do some static define instead but I don't think it will work.

@byroot byroot force-pushed the ruby-deprecation-warnings branch 2 times, most recently from f6cd89e to c5b0fd8 Compare December 17, 2024 10:03
@byroot
Copy link
Contributor Author

byroot commented Dec 17, 2024

Alright, I tried a few workarounds but they're all awful and don't pass the coverage check anyway, so I'll wait for instruction from the maintainers.

@byroot byroot force-pushed the ruby-deprecation-warnings branch 4 times, most recently from 1a11009 to 5d61ea4 Compare December 17, 2024 10:58
@byroot
Copy link
Contributor Author

byroot commented Dec 17, 2024

Alright, I believe the remaining errors are unrelated to my changes?

# Set Ruby deprecation warnings on or off.
def deprecation_warnings=(value)
@deprecation_warnings_set = true
::Warning[:deprecated] = value
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd rather we overload warnings= here to be one of: false :deprecations_only :all or true where true has the same effect as :all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suppose that make sense, one downside I can think of however if that since regular warnings are quite verbose, it's likely that a bunch of projects have warnings = false and won't ever know about deprecation warnings.

So while I agree making it a difference config is a bit less clean, it give every RSpec user the opportunity to know about it.

But that's your call.

Copy link
Member

Choose a reason for hiding this comment

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

I think that risk is low as false was the default, but I propose that the setting be :none, :deprecations_only and :all with true/false synonyms for :all/:none that in turn issue an RSpec deprecation warning asking people to update to the specifics.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. Ok, I'll try to implement that then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I think I implemented what you suggested. I'm not really familiar with RSpec codebase though, so hopefully it's implemented the proper way?

Copy link
Member

Choose a reason for hiding this comment

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

Last change, can we wrap this up into the above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry, I don't think I understand what you mean by that.

@byroot byroot force-pushed the ruby-deprecation-warnings branch from 5d61ea4 to d13009c Compare December 17, 2024 12:41
@benoittgt
Copy link
Member

benoittgt commented Dec 23, 2024

Hello Jean

I see two failures in feature specs.

bundle exec cucumber features/configuration/enable_global_dsl.feature:40
bundle exec cucumber features/command_line/ruby.feature:10

It happens in the CI after the simplecove coverage tests...

The issue seems related to how we try to report caller lines when using .warning. Maybe @JonRowe knows if .warning should be used here. I'm tempted to change how we report caller line. In my test I have 14 frames and with the current setup we are getting outside of frames before looping 3 times. 😄

Quick and dirty.

--- a/rspec-support/lib/rspec/support/caller_filter.rb
+++ b/rspec-support/lib/rspec/support/caller_filter.rb
@@ -65,7 +65,7 @@ module RSpec
 
         loop do
           stack = caller_locations(skip_frames, increment)
-          raise "No non-lib lines in stack" unless stack
+          break unless stack

Snippet to reproduce.

require "bundler/inline"
gemfile(true) do
  source "https://rubygems.org"
  gem 'rspec', path: '../rspec'
end

require 'rspec/autorun'

RSpec.describe 'Joyeux Noël' do
  it do
    expect({ :a => 1, :b => 2 }).not_to include(:a, :b)
  end
end

@byroot byroot force-pushed the ruby-deprecation-warnings branch from e835fa6 to 1e1fd1d Compare December 27, 2024 12:27
@byroot
Copy link
Contributor Author

byroot commented Dec 27, 2024

I fixed the typos in the message.

For the remaining failures, I don't know what to do, so I'll await further instructions.

@byroot byroot force-pushed the ruby-deprecation-warnings branch from 1e1fd1d to f654aaa Compare December 27, 2024 14:47
@byroot byroot force-pushed the ruby-deprecation-warnings branch from c836b7c to 5aec383 Compare December 27, 2024 14:50
@byroot byroot force-pushed the ruby-deprecation-warnings branch from 5aec383 to fc98e40 Compare December 28, 2024 20:00
@byroot
Copy link
Contributor Author

byroot commented Dec 28, 2024

The remaining failures are what @benoittgt pointed out.

Both failing scenarios are related to rspec/autorun:

Failing Scenarios:
cucumber features/command_line/ruby.feature:10 # Scenario: Require `rspec/autorun` from a spec file
cucumber features/configuration/enable_global_dsl.feature:40 # Scenario: By default rspec/autorun allows the DSL to be used globally

It fails because it's emitting a warning about config.warnings not being set.

@byroot
Copy link
Contributor Author

byroot commented Dec 28, 2024

Thanks for the fix, I was a bit stuck because of my limited knowledge of rspec.

One of the spec I added now complains but I can fix that.

@byroot byroot force-pushed the ruby-deprecation-warnings branch from 9615fb1 to 94f50cf Compare December 28, 2024 20:53
@pirj
Copy link
Member

pirj commented Dec 28, 2024

That one seemed flaky and caused by order dependence (/tmp/aruba isn't getting created when `rspec spec/integration/order_spec.rb is run). But the seemingly trivial fix, mocking other similar specs that use Aruba, didn't work. I can reproduce this locally. PS fixed. PPS merged in #176

I don't mind reverting my last commit and merging this PR as is, as this failure seems largely unrelated to this PR. @JonRowe WDYT?
But if it's a quick fix, I hope you don't mind including it. I can extract the flaky fix.

My apologies for the inconvenience, we've just changed the repo structure dramatically, and it may have some rough edges

@pirj pirj force-pushed the ruby-deprecation-warnings branch from bad2075 to 496c493 Compare December 28, 2024 21:32
@pirj
Copy link
Member

pirj commented Dec 28, 2024

💚

Copy link
Member

@benoittgt benoittgt left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks !

@JonRowe
Copy link
Member

JonRowe commented Jan 23, 2025

Apologies for the delay in getting back to this some build failures and work have sucked up all my free time, its on my list to get this in this week.

@JonRowe
Copy link
Member

JonRowe commented Jan 23, 2025

Can we also back out the unrelated changes for the other specs, @pirj if you'd like to work on another PR for those I'm happy to review that.

@pirj pirj force-pushed the ruby-deprecation-warnings branch 2 times, most recently from 94f50cf to 6d351b1 Compare January 23, 2025 12:55
@pirj
Copy link
Member

pirj commented Jan 23, 2025

@JonRowe Done

byroot added 2 commits March 18, 2025 18:36
Fix: rspec#37

Since Ruby 2.7.2, Ruby expect test frameworks to enable deprecation
warnings so users get advance notice of future breaking changes.
@pirj pirj force-pushed the ruby-deprecation-warnings branch from 6d351b1 to cf7298c Compare March 18, 2025 15:36
@pirj
Copy link
Member

pirj commented Mar 18, 2025

RuboCop job fix #201

HunkGenerator is flaky

@JonRowe is the rest good to merge?

@JonRowe
Copy link
Member

JonRowe commented Mar 18, 2025

Sorry for the silence on this, I'm going to merge this manually when I get the time to split it into 3.x and 4.x capable patches, but no input is needed beyond that.

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.

Warning[:deprecated] = true by default in RSpec
5 participants