Code review comment for ~lucaskanashiro/ubuntu/+source/vagrant:fix-warnings

Revision history for this message
Bryce Harrington (bryce) wrote :

A few bits of advice on SRUs (nothing needing changed for this one, but to keep in mind next time):

With the Potential Regression section, one thing I try to do beyond just describing the *likelihood* of a regression, is to think about what a regression would look like. Because, of course the potential should always be Low - if it were high you obviously wouldn't consider SRUing the bug. But a more realistic scenario is that someone updates to your fix, and something completely unrelated breaks, and they wonder if your fix caused their break. By describing what a regression would look like, you can give bug reporters and triagers some clue that actually no, their problem is unlikely to be Lucas' fault, go look elsewhere. ;-)

I've also found by thinking from this direction once and a while I realize there was some angle to the bug I didn't adequately cover, and thus ended up with a better fix overall. So it's a good practice. I think Martin Pitt taught me this way back when.

Also, in test cases, I like to also include directions to install the fixed version, and then show what the output is in that case, for comparison. For this bug the test case is simple enough that's not important. One thing I might do is to trim the usage output, since its text is unnecessary for validating the bug, e.g.:

# Broken behavior:
$ apt install vagrant
$ vagrant --help > /dev/null
NOTE: Gem::Specification.default_specifications_dir is deprecated; use Gem.default_specifications_dir instead. It will be removed on or after 2020-02-01.
Gem::Specification.default_specifications_dir called from /usr/share/rubygems-integration/all/gems/vagrant-2.2.6/lib/vagrant/bundler.rb:428.
/usr/share/rubygems-integration/all/gems/vagrant-2.2.6/lib/vagrant/errors.rb:103: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/usr/share/rubygems-integration/all/gems/i18n-1.8.2/lib/i18n.rb:195: warning: The called method `t' is defined here
(eval):3: warning: Using the last argument as keyword parameters is deprecated; maybe ** should be added to the call
/usr/share/rubygems-integration/all/gems/vagrant-2.2.6/lib/vagrant/ui.rb:223: warning: The called method `say' is defined here
$

# Install fixed version
$ sudo add-apt-repository -yus ppa:lucaskanashiro/focal-vagrant-sru
$ sudo apt install kafka
[...]

# Fixed behavior
$ vagrant --help > /dev/null
$

My rationale for being a bit pedagogic with the test case is because the more "paint by numbers" it is, and easier it is to use, the more likely someone else will bother spending time validating the fix, and the more smoothly and swiftly it'll go through the SRU process. It also can help future bug reporters and triagers to quickly check that their unrelated issue is not caused by this fix.

« Back to merge proposal