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.

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

Clean up console warnings when running tests #1933

Merged
merged 16 commits into from
Dec 15, 2023
Merged

Clean up console warnings when running tests #1933

merged 16 commits into from
Dec 15, 2023

Conversation

joelhawksley
Copy link
Member

What are you trying to accomplish?

This PR resolves and/or hides the warnings shown while running tests.

What approach did you choose and why?

I did the best I could to resolve warnings from within the framework. We were printing warnings from Rails and other dependencies which just created noise in the console.

Anything you want to highlight for special attention from reviewers?

Along the way, I also found a way to have YARD not build the docs HTML, which should save us time in our build processes!

Rakefile Outdated
@@ -9,6 +9,7 @@ Rake::TestTask.new(:test) do |t|
t.libs << "test"
t.libs << "lib"
t.test_files = FileList["test/**/*_test.rb"]
t.warning = false
Copy link
Contributor

Choose a reason for hiding this comment

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

Which warnings is this suppressing? Should we fix these instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

These three:

/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.8/lib/active_support/core_ext/time/deprecated_conversions.rb:42: warning: method redefined; discarding old to_s
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/activesupport-7.0.8/lib/active_support/time_with_zone.rb:210: warning: previous definition of to_s was here
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/zeitwerk-2.6.11/lib/zeitwerk/kernel.rb:38: warning: /Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/zeitwerk-2.6.11/lib/zeitwerk/kernel.rb:38: warning: loading in progress, circular require considered harmful - /Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/yard-0.9.34/lib/yard.rb
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/date_time_parser.rb:837: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/date_time_parser.rb:691: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32457: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32618: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32654: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32743: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32759: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32825: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32866: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:32891: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/address_lists_parser.rb:31987: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_disposition_parser.rb:795: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_disposition_parser.rb:819: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_disposition_parser.rb:586: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_location_parser.rb:761: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_location_parser.rb:592: warning: assigned but unused variable - disp_type_s
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_location_parser.rb:607: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_transfer_encoding_parser.rb:358: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_type_parser.rb:942: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_type_parser.rb:966: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/content_type_parser.rb:712: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/envelope_from_parser.rb:3241: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/message_ids_parser.rb:5112: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/message_ids_parser.rb:4848: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/mime_version_parser.rb:322: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/phrase_lists_parser.rb:702: warning: assigned but unused variable - testEof
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:7861: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8058: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8091: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8108: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8215: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8313: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8329: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8345: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8419: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8437: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8455: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8541: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8568: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8614: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8647: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8665: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8685: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8707: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:8727: warning: statement not reached
/Users/joelhawksley/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/gems/mail-2.8.1/lib/mail/parsers/received_parser.rb:7514: warning: assigned but unused variable - testEof

I don't see an obvious resolution for any of them.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's unfortunate since I'd like to still see warnings for ViewComponent related code. I did find https://github.com/jeremyevans/ruby-warning but not sure if it's worth the effort. Thanks for the context!

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh how cool! ruby-warning was totally worth the minimal effort. Just pushed the change ❤️

@@ -42,7 +43,10 @@ end
namespace :docs do
# Build api.md documentation page from YARD comments.
task :build do
YARD::Rake::YardocTask.new
YARD::Rake::YardocTask.new do |t|
t.options = ["--no-output"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!


# Use https://github.com/jeremyevans/ruby-warning
# to restrict warnings outside our control
["mail", "activesupport", "yard"].each do |gem_name|
Copy link
Contributor

@BlakeWilliams BlakeWilliams Dec 15, 2023

Choose a reason for hiding this comment

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

I also saw this in the README if we wanted to ignore all gems, but no strong feelings:

# Ignore all warnings in Gem dependencies
Gem.path.each do |path|
  Warning.ignore(//, path)
end

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking would be good to ignore as little as possible to start as I'm worried about missing legitimate warnings.

@joelhawksley joelhawksley merged commit 1b3747b into main Dec 15, 2023
32 of 33 checks passed
@joelhawksley joelhawksley deleted the warnings branch December 15, 2023 23:13
claudiob pushed a commit to claudiob/view_component that referenced this pull request Jan 3, 2024
* use Ruby 3.2.2

* update yard dev dependency to latest

* Reduce noise in tests by turning off warnings

* appease YARD warning for including Rails routes, which are dynamic

* Resolve assert_nil warning from Minitest

* fix param name error in YARD comment

* silence YARD output in tests

* Fix YARD errors

* add CHANGELOG entry

* appease Standard

* include sha in artifact upload filename

* Turn off warnings for specific dependencies

* standard

* lock to pre-v4 upload-artifact action

* fix version number

* use specific version of checkout action

---------

Co-authored-by: Joel Hawksley <[email protected]>
Co-authored-by: Joel Hawksley <[email protected]>
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.

2 participants