[RFC] "compact" deprecation lists

[davidstosik]

What

Currently, the YAML files produced by deprecation toolkit will contain hundreds of identical deprecations produced by a single test. The reason is rooted in Collector's implementation.

In this RFC I'd like to suggest a more compact format.

Why

I find the volume of these deprecation files daunting, and because deprecations are not sorted, it can be difficult to evaluate whether a new deprecation is completely new within a test, or "one more" in a long list of deprecations that were already produced by that test.

The same volume will produce some noise in pull requests, unnecessarily inflating the diff count (which at least might have a psychological effect), and potentially making it hard to scroll in a code diff.

How

My proposition is to change for a format where, instead of repeating identical deprecations, we count them. The format could look something like this:

---
test_my_test_title:
- deprecation: 'DEPRECATION WARNING: Don''t use bla bla'
  count: 1
- deprecation: 'DEPRECATION WARNING: Don''t do this'
  count: 87

I quickly tried writing a patch in an existing app, and it looked like this. (Click to open.)

require "deprecation_toolkit/read_write"

# I limited my changes to the YAML read/write logic, but this could be more efficient.

module DeprecationToolkit
  module ReadWriteHelper
    def read(test)
      deprecation_file = Bundler.root.join(recorded_deprecations_path(test))
      data = YAML.load(deprecation_file.read).fetch(test_name(test), [])
      if data.first.is_a?(Hash)
        data.flat_map do |hash|
          [hash["deprecation"]] * hash["count"]
        end
      else # for backwards compatibility
        data
      end
    rescue Errno::ENOENT
      []
    end

    def write(deprecation_file, deprecations_to_record)
      create_deprecation_file(deprecation_file) unless deprecation_file.exist?

      content = YAML.load_file(deprecation_file)

      deprecations_to_record.each do |test, deprecations|
        if deprecations.any?
          content[test] = deprecations.tally.sort.map do |deprecation, count|
            {
              "deprecation" => deprecation,
              "count" => count,
            }
          end
        else
          content.delete(test)
        end
      end

      if content.any?
        deprecation_file.write(YAML.dump(content))
      else
        deprecation_file.delete
      end
    end
  end
end

Anything else

I guess this is really not a big deal, but somehow I found myself annoyed by these huge lists and thought I'd spend a bit of time understanding and thinking about a possible improvement.

I noticed that the current logic reads and rewrites a given test file's deprecation list YAML after each test in the file is completed. In big test suites where single test files contain a lot of tests, and lots of repeated deprecations, this could have a slight beneficial impact on file read/write time. (Less stuff to read/write every time.)

I also wrote a script that could update all deprecation list files to the new format, it's rather trivial.

require "yaml"

Dir["test/deprecations/**/*.yml"].each do |file|
  compacted = YAML.load_file(file).transform_values do |deprecations|
    deprecations.tally.sort.map do |deprecation, count|
      {
        "deprecation" => deprecation,
        "count" => count,
      }
    end
  end
  File.write(file, YAML.dump(compacted))
end

I haven't run the test suite on this change, so it is possible it needs more work to prevent breaking anything.

Before putting more work in, I'd like to hear opinions. 🙏🏻

[mltsy]

I'm new to this gem, but this is one of the things that is bothering me right off the bat, so I really like this idea.

I had a different instinct on how to compact the list of deprecations for my use case though, which was to organize them by deprecated source location rather than test file/name. That seems like a much more actionable list to have (here's all the deprecated code in this file, so you can fix them all at once). But maybe there are reasons that would be more complex to implement, or less ideal for some use cases. (?)

[brkn]

Any chance of reviving this RFC?

When test order is randomized, record behaviour shuffles the same fixture file's entries unnecessarily. Shuffling the tests also can cause the offending line number to be changed because of rspec's DSL helpers.

This is neither efficient nor ergonomic for the pr authors.

About the new compact format

May I suggest using the rubocop_todo.yaml format.

# Offense count: 12
# This cop supports safe autocorrection (--autocorrect).
# Configuration parameters: Max, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns.
# URISchemes: http, https
Layout/LineLength:
  Exclude:
    - 'app/controllers/authors_controller.rb'
    - 'app/controllers/books_controller.rb'

# Offense count: 12
# This cop supports safe autocorrection (--autocorrect).
# Configuration parameters: Max, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns.
# URISchemes: http, https
Metrics/AbcSize:
  Exclude:
    - 'app/controllers/authors_controller.rb'
    - 'app/controllers/books_controller.rb'

Sh*tlist for deprecations can also live in a single compact yaml file in the root. Maybe like this

# Offense count: 12
DEPRECATION WARNING: WARNING: `allow(...).to receive(..).ordered` is not supported and will have no effect, use `and_return(*ordered_values)` instead..:
  Offending files:
    - [RAILS_ROOT]/spec/requests/articles_spec.rb
    - [RAILS_ROOT]/spec/requests/books_spec.rb
  
# Offense count: 3
DEPRECATION WARNING: warning: rb_check_safe_obj will be removed in Ruby 3.0:
  Offending files:
    - [RUBY_LIBS_DIR]/psych/visitors/to_ruby.rb:74

[Edouard-chin]

Thanks for the RFC and the discussions. I just wanted to provide some context on the original decision to have this very verbose deprecation lists.

This tool was both built to catch and record deprecations but also to ensure that the deprecations were thrown in a deterministic order. This decision lead us at Shopify to detecting a lot of flaky tests in our test suites because the recorded deprecation orders wouldn't match further test runs.

I'm happy to revisit this but I figured it was worth providing some context as to why this is like this :).