Avoiding Cognitive Overload
Tests For Productivity and Education

Make sure all task references are valid

In our core application we had an issue where a migration would reference an nonexistant task.

What was happening is that user A would make a PR referencing MyTask, but user B would delete task MyTask because they noticed it was no longer used when they finished their use of it.

While this is a valid train of thought, the occurence of events would be such that we did not catch the mistake until the migration was running in production. To fix this I wrote a test to make sure all tasks that are referenced exist as expected

The Test

    
  test 'migrations dont reference invalid tasks' do
    # This helper iterates all files in the project looking for `*_task.rb` files
    available_tasks = DevelopmentSupport::Rake::MaintenanceHelpers.maintainance_task_files
    # This helper looks through all of our migrations to look for references to `*Task`
    used_tasks = DevelopmentSupport::Rake::MaintenanceHelpers.used_tasks

    # If we consider that all `*Task` references should find a definition at *_task.rb`
    # then we can make sure this is true
    used_tasks.each do |task|
      path_to_file = Pathname.new(task[:caller_file]).relative_path_from(Rails.root)
      msg = <<~EOF
      This File \x1b[36m#{path_to_file}\x1b[0m\n"\
      References: \x1b[36m#{task[:name]}\x1b[0m\n"\
      But expected defintion for that reference did not exist in db/maintenance/maintenance.
      Please make sure this task is defined at the expected location:  \x1b[36m#{task[:file_name]}\x1b[0m
      EOF
      assert available_tasks.include?(task[:file_name]), message: msg
    end
  end
    
  

Make sure we don't add back gems

Sometimes we remove a gem from the code base for various reasons.

With a larger org, it's hard to get that message out to people, so I wrote a test to check that some gems aren't added back.

Below is an example of that test

The Test

    
    test "no removed gems are added to the Gemfile" do
      base_msg = <<~EOF
        Context
        =========
        When we add a gem, we need to really look and understand what we are adding.
        We need to ask if we are adding a non trivial amount of code for a very small amount of benefit (code used).
        Each gem added means that we will be adding a significant amount of code and in turn we will be increasing boot time,
        application performance, and increase the likelihood of a Ruby or Rails upgrade experiencing problems
        (since not all gems update to the point where we need them to).
        This test is here to make sure we don't add gems we have deemed:
        - generally not much of the code used
        - provides wrong functionality
        - not supported upstream anymore
        - etc
        The follow explains what gems are being added, and shouldn't be - and why they shouldn't be added.
      EOF

      # Load a list of gems we have removed
      removed_gems = YAML.load_file(Rails.root.join('test/files/removed_gems.yml'))

      # Find a list of all gems currently in the project
      definition = Bundler.definition
      gem_names = definition.specs.collect(&:name)

      # Get the intersection of removed gems with the current gem names
      should_not_be_added = removed_gems.keys & gem_names

      # If we added gems we shouldn't have, collect those into a message list
      messages = should_not_be_added.each_with_object([]) do |gem_name, msgs|
        msgs << "#{gem_name}\n#{'=' * gem_name.length}\n#{removed_gems[gem_name]}"
      end

      # Fail if we ended up with messages (aka added removed gems)
      assert messages.empty?, message: ([base_msg] + messages).join("\n\n")
    end

    
  

The Removed Gems

    
  # This file documents why we removed gems and fails a test if someone adds it back
  # The message should explain why it was removed and what we need to prove if someone wants to add it back
  #
  # Format gem: Reason it was removed
  #
  faker: |
    - The faker gem is extremely large and complex, and we used such a small amount of it.
    - Faker made a lot of very poor assumptions in the way it calculated positional data (zip codes, addresses)
      - this means that the data it gives is invalid which meant that we failed our own tests and validations. 
      - This made our development pipeline flaky and prevented hundreds of engineers from working smoothly. 
      - Our test pipeline also experienced issues.
    We removed faker for these reasons, please do not add it back without validating that it gives:
      - Proper zip and postal codes that pass our validation 100% of the time. Faker just uses format, but doesn't provide real data.
      - Assure that for every field you intend to generate fake data for, faker generates data that will always validate against our own validation rules.
      - We use a significant portion of the gem
      - The functionality isn't already in ShopBuilder classes
  rspec: |
    We have standardized on one Ruby test framework for Shopify core for organizational efficiency: Minitest.
    - Standardizing on one tool will make it easier for people to move between codebases and be up to speed quickly.
    - We have build a lot of tooling around Minitest to maintain the quality of our test suite, which will not work for RSpec.
      Migrating those tools to RSpec will be a significant amount of work.
    
  

Ban Git-based Gems

Git-based gems can cause a lot of pain when it comes to performance and authentication.

They also make it hard to use pre-compiled images for native extensions.

We decided to ban git gems, so we wrote a test to fail on newly added git gems but not on old ones

The Test

    
  # Rails is a gem that we are always trying the git version, even to get bug fixes early or even to test the next
  # releases. Becasuse of that it is better to always whitelist it.
  PERMANENT_ALLOWLIST = %w(
    https://github.com/rails/rails.git
    https://github.com/rails/activeresource.git
  )

  test "git gems are all on the deprecation list" do
    # Find all the bundler dependencies, then only get the ones that are using git
    definition = Bundler.definition
    sources = definition.dependencies.collect(&:source).compact
    sources.select! { |s| s.is_a?(Bundler::Source::Git) }

    # Remove any gems we want to be using git and load the deprecation list
    # A deprecation list is a list of things that violate a new rule. This list is used to allow those violations
    # which gives us time to migrate and fix the new rule without added new violations.
    git_urls = sources.collect(&:uri).sort - PERMANENT_ALLOWLIST
    expected_urls = File.readlines(Rails.root.join('test/files/git_gems_deprecation_list.txt')).map(&:chomp)

    # For each git url in our gemfile, check that it is expected to be loaded
    # A url is expecte to be loaded if it is on the deprecation list
    # If it is not, then tell the user what is wrong (they added the gem), why it is wrong (performance and development issues), and how they can continue forward (using a private gem host).
    git_urls.each do |git_url|
      assert expected_urls.include?(git_url), message: <<~EOF
        It seems you have added a Git-Based Gem (#{git_url}) to our Gemfile. This is now deprecated.
        Git Gems are being phased out of all development at Shopify.
        They are not performant and cause a lot of issues.
        There is more information at [PROJECT INFORMATION]
        To continue forward, please use a non-git source.
        [PRIVATE GEM HOST] can be used for private gems.
        Please read [LINK TO DOCUMENTATION] for help on setting that up
      EOF
    end

    # If a user removed a git gem, make sure the deprecation list is updated.
    # In this case, we ask that the user updates the list themselves and thank them for being
    # a good person. This allows us to share the work and make sure the list doesn't become stale.
    expected_urls.each do |expected_url|
      assert git_urls.include?(expected_url), message: <<~EOF
        It seems you removed a git url (#{expected_url}) from our app.
        Congratulations! You are a steward and a fanstastic person.
        Git gems cause us a lot of pain in performance and in issues on dev laptops
        To track this, a deprecation list was added at test/files/git_gems_deprecation list.txt
        Please remove #{expected_url} from it
      EOF
    end
  end