Shopify/packwerk

Extract constant names from fixtures

wildmaples opened this issue · 1 comments

What?

In test files, when we see:

class MyTest < ActiveSupport::TestCase
  setup do
    @thing = things(:thing_fixture)
  end
end

We want to extract the constant name ::Thing from the call things(:thing_fixture).

Why?

Tests should also be appropriately packaged (in other words, no boundary violations), and fixtures are an easy way for packages to (currently) depend on other packages without packwerk realizing. By registering these constant references we have a better signal to help packwerkers have proper isolation for their tests.

How?

I'm thinking this will be a few PRs:

  1. Introduce an interface for getting a constant name from a node.
  2. Refactor ReferenceExtractor to accept a list of instances of anything that implements the "constant name from node" interface in (1).
  3. Extract existing "constant name from node" from ReferenceExtractor (the node.const_node? path).
  4. Add new class to get constant name from a node if the node looks like a fixtures call.

For (4), to reduce false positives, I'm thinking the criteria for a "fixtures function call" would be the following:

  • Directory of the file being parsed matches **/test/**/*_test.rb or **/spec/**/*_spec.rb
  • AST node is of type :send with one or more arguments that are all symbols, and
  • Name of function maps to a file in (a configurable) fixtures path.
    • We can preload all fixtures files to get the model class name too (via _fixture.model_class, if it exists, otherwise just use the inflector on the filename).

cc: @thegedge

We tried to add fixture detection into Packwerk because it is responsible for ensuring that code has no boundary violations. During the implementation process, I noticed that this approach is tying Packwerk directly with a fixture implementation. To detect the fixtures, Packwerk has to assume the name of a fixture method, how many arguments it takes, and what type of arguments.

# s(:send, nil, :shop,
#   s(:sym, :snowdevil))
sig { params(node: ::AST::Node).returns(T::Boolean) }
def potential_fixture_call?(node)
   arguments = Node.method_arguments(node)
   arguments.length == 1 && Node.symbol?(arguments[0])
end

If implementing the fixture detection in Packwerk, we would have to read out the fixture to make sure it doesn’t redefine the model class. Core, in particular, adds additional overhead because we define our fixture across multiple folders. To add to that, the same fixtures get defined across different folders as well, which adds additional complexity. In my implementation, which wasn't complete yet, it boiled down to a performance difference of 12% where the current time to complete is around eight minutes and the time to complete with the fixture detection is around nine minutes.

Those circumstances suggest that we were trying to solve the issue from the wrong direction. With our current knowledge, the likely issue lies in how we implemented our fixtures. This will happen in an internal discussion.