enkessler / cuke_cataloger

A handy gem for adding identifier tags to Cucumber tests.
MIT License
12 stars 4 forks source link

incremental Id shortcomings in bigger projects #5

Open alexanderjeurissen opened 3 years ago

alexanderjeurissen commented 3 years ago

cuke_cataloger works great but there are a couple of shortcomings that make it hard to use in bigger repositories or in repositories with a lot of contributors / feature branches.

Problem 1: Incremental ID requires processing all existing tags

Currently the cuke_cataloger:tag_tests rake task is very slow, on average in a codebase that I maintain where we have ~3000 tagged scenarios it takes on average ~20-30 seconds to complete tagging.

One of the causes of this slow execution time is the need to scan through all feature_files / scenarios to:

  1. check if the feature contains untagged scenarios
  2. determine the current max test_case id.

Ideally generating a unique identifier would not require processing all files, but rather be derived from a given scenario itself. For instance by creating a hex digest from a unique combination of scenario attributes.

This would allow for only processing or tagging specific files (for instance in a git hook only tag changed feature files) which significantly improves performance.

As I want to enforce engineers to tag their new tests, the ability for this to be executed without much overhead as part of a git hook would be a big gain.

Problem 2: Incremental ID very prone to duplicates on merge

Consider the following branches:

image

Both branches contain a new feature file with a single scenario. Running bundle exec rake cuke_cataloger:tag_tests in each respective feature_branch will tag the new scenario with test_case_1 given that cuke_cataloger is not aware of other tags in other branches.

As a result when merging both feature branches to main we will end up with two scenarios with the same tag.

In a project where I recently introduced cuke_cataloger this poses a real problem. We run cuke_cataloger:validate_tests as a build step on our CI to ensure the uniqueness of the test_case_x tags as we rely on the uniqueness to store structured logging. In the scenario above this will result in build failures on the main branch which is undesirable as it blocks other engineers from kicking off a code deploy.

Solution

Given the above two problems, currently I cant rely on cuke_cataloger and wrote a custom script that incorporates parts of cuke_cataloger combined with cuke_slicer:

#!/usr/bin/env ruby
# frozen_string_literal: true

require 'cuke_slicer'
require 'pastel'
require 'digest/md5'

class CucumberTagger
  TAG_PREFIX ||= '@test_case_'

  def tag_test(test, tag)
    feature_file = test.get_ancestor(:feature_file)
    file_path = feature_file.path

    file_lines = File.readlines(file_path)

    index_adjustment = @file_line_increases[file_path]

    adjacent_tag_line = test.source_line - 1

    test_index = adjacent_tag_line + index_adjustment

    indentation = file_lines[test_index][/^\s*/].length
    padding_string = (' ' * indentation)

    file_lines.insert(test_index, "#{padding_string}#{tag}\n")

    File.open(file_path, 'w') { |file| file.print file_lines.join }

    @file_line_increases[file_path] += 1
  end

  # NOTE: Use the slicer to find all tests
  # We extract the test_objects so we get the CukeModeler representation of
  # the test_cases.
  def test_cases
    @test_cases ||= begin
      # NOTE: if ARGV is empty return all scenarios in the features folder
      return CukeSlicer::Slicer.new.slice('features/', {}, :test_object) if ARGV.empty?

      found_tests = []
      ARGV.each do |target|
        found_tests.concat CukeSlicer::Slicer.new.slice(target, {}, :test_object)
      end

      found_tests
    end
  end

  # NOTE: remove test_cases that already have a test_case tag
  def untagged_test_cases
    @untagged_test_cases ||= test_cases.keep_if do |test_case|
      next true if test_case.tags.empty?

      # NOTE: search collection of tags for one that matches the pattern
      # if it does the return value is not nil which will result in the test_case being removed
      test_case.tags.detect do |tag|
        tag.name.start_with?(TAG_PREFIX)
      end.nil?
    end
  end

  def initialize
    @file_line_increases = Hash.new(0)

    pastel = Pastel.new(enabled: true)

    puts pastel.on_blue.bold.white("[CUKE_CATALOGER] tagging #{untagged_test_cases.count} "\
                                   'test_cases..')

    # NOTE: tag test_cases that are untagged
    untagged_test_cases.each do |test_case|
      feature_file = test_case.get_ancestor(:feature_file)
      file_path = feature_file.path

      # NOTE: construct array of content that we'll include in the hex digest
      # - file_path
      # - source_line
      # - string representation of test_case
      # - Timestamp
      digest_content = []
      digest_content << file_path
      digest_content << test_case.source_line
      digest_content << test_case.to_s
      digest_content << Time.now.utc.to_i

      tag = "#{TAG_PREFIX}#{Digest::MD5.hexdigest(digest_content.join)}"

      tag_test(test_case, tag)
    end

    puts pastel.on_green.bold.black('[CUKE_CATALOGER] finished tagging test_cases')
  end
end

CucumberTagger.new

It does not support all use-cases that cuke_cataloger supports for instance it does not include sub-id but I think it gets the idea across. The above script only takes ~6 seconds on ~3000 scenarios instead of the aforementioned ~20 seconds.

It would be great if we could incorporate parts of this script upstream as I'd rather use cuke_cataloger then having yet another custom script to maintain ;-)

enkessler commented 3 years ago

it takes on average ~20-30 seconds to complete tagging

That doesn't sound overly onerous unless you're running the tagger all the time. Which you aren't meant to be doing.

Ideally generating a unique identifier would not require processing all files, but rather be derived from a given scenario itself. For instance by creating a hex digest from a unique combination of scenario attributes.

With incremental IDs Me: "Hey, co-worker, which test was it that failed?" Co-worker: "Test '3752'." Me: "Thanks!"

With hex IDs Me: "Hey, co-worker, which test was it that failed?" Co-worker: "Test 'C8EDEA22AF2F8BA51438221440D47DFA'." Me: "..." Me: "You know what, just email it to me."

So, while I wouldn't object to offering an additional type of tagging scheme or allowing for entirely arbitrary ones, the basic numbering option probably is probably going to stick around as the default. Sure, a tagging solution that was faster would save time, but human time is more valuable than computer time and numbers are more human time friendly.


Regarding your merge conflict problem, do the tests really need cataloging immediately when they are written or even while they are still on whatever temporary side branch a developer is working on? If cataloging can wait until the branch is merged into the main development branch then cataloging could be an automatic post-merge action in Git or your CI.

Alternatively, you could store the current maximum ID number (in your database or whatever part of your development infrastructure is convenient) and then have your cataloging script grab that stored value and give it to the cataloger as an index starting point (via the explicit_indexes parameter. The script would also update the saved index to the new highest ID once the cataloging was done. This approach would work both as an 'on demand' scrip that a developer could run locally or as a CI job or Git hook. Personally, I recommend having it be a CI job/hook because there is still the chance of ID overlap if more than once instance of the script was running at the same time.