Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not allow to use the RSpec tag option together with the RSpec split by test examples feature #139

Merged
merged 15 commits into from
Apr 20, 2021

Conversation

ArturT
Copy link
Member

@ArturT ArturT commented Apr 8, 2021

Do you pass RSpec option --tag in command like bundle exec rake "knapsack_pro:queue:rspec[--tag mytag]"?
If you do it and you use at the same time KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true then --tag option might not be respected by RSpec due to a bug in the RSpec. RSpec has a higher priority to run test examples like spec/a_spec.rb[1:1] than respect the --tag option. This can result in running test cases that you didn't intend to run.

You should not use the --tag option when you use RSpec split by test examples feature KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true.

See more:
https://knapsackpro.com/faq/question/how-to-split-slow-rspec-test-files-by-test-examples-by-individual-it#warning-dont-use-rspec-tag-option

@ArturT ArturT added the bug label Apr 8, 2021
@ArturT ArturT requested a review from shadre April 8, 2021 19:31
Copy link
Member

@shadre shadre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we improve the error messages gramatically

@ArturT ArturT changed the title Do not allow to use the RSpec tag option at the same time with RSpec split by test examples feature Do not allow to use the RSpec tag option together with the RSpec split by test examples feature Apr 18, 2021
@ArturT
Copy link
Member Author

ArturT commented Apr 18, 2021

@shadre Thanks for the review. I've applied suggestions. Please let me know if we are good to merge this PR.

@ArturT ArturT requested a review from shadre April 18, 2021 13:57
Copy link
Member

@shadre shadre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks great to me 👍

@ArturT ArturT merged commit 1e811dc into master Apr 20, 2021
@ArturT ArturT deleted the rspec-tag-exception branch April 20, 2021 20:31
ArturT added a commit to KnapsackPro/rails-app-with-knapsack_pro that referenced this pull request Dec 28, 2023
ArturT added a commit that referenced this pull request Dec 28, 2023
@ArturT
Copy link
Member Author

ArturT commented Dec 28, 2023

tech note

About the following issue:

RSpec has a higher priority to run test examples like spec/a_spec.rb[1:1] than respect the --tag option. This can result in running test cases that you didn't intend to run.

To reproduce the issue make changes:

In the knapsack_pro gem:

# lib/knapsack_pro/queue_allocator.rb
    def test_file_paths(can_initialize_queue, executed_test_files)
      @@index ||= 0
      batches = [
        ['spec/features/calculator_spec.rb[1:2:1]', 'spec/controllers/articles_controller_spec.rb'],
        ['spec/collection_spec.rb'],
      ]
      tests = batches[@@index] || []
      puts 'INSPECT:'
      puts tests.inspect
      @@index += 1
      return tests
      
      # the rest of the code that is ignored thanks to the above `return tests`
      return [] if @fallback_activated
      # ...

In the rails-app-with-knapsack_pro repo:

Run the bin script: bin/knapsack_pro_queue_rspec_record_first_run with modified content as following to run tagged tests.

# bin/knapsack_pro_queue_rspec_record_first_run

RANDOM_CI_BUILD_ID=$(openssl rand -base64 32)
CI_BUILD_ID=${4:-$RANDOM_CI_BUILD_ID}
COMMIT_HASH=$(ruby -e "require 'securerandom'; puts SecureRandom.hex")
BRANCH_NAME=fake-branch

# you can set a const fake data if you need to test running tests on 2 CI nodes for the same commit/branch
#COMMIT_HASH=57dccc53cb2ee921692ad1d3df971674

#export KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true

#export EXTRA_TEST_FILES_DELAY=10 # seconds
# uncomment the following line to verify if Knapsack Pro raises an exception preventing the RSpec --tag usage
#export KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true

KNAPSACK_PRO_ENDPOINT=http://api.knapsackpro.test:3000 \
  KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC=fec3c641a3c4d2e720fe1b6d9dd780bc \
  KNAPSACK_PRO_CI_NODE_BUILD_ID=$CI_BUILD_ID \
  KNAPSACK_PRO_COMMIT_HASH=${3:-$COMMIT_HASH} \
  KNAPSACK_PRO_BRANCH=$BRANCH_NAME \
  KNAPSACK_PRO_CI_NODE_TOTAL=${2:-2} \
  KNAPSACK_PRO_CI_NODE_INDEX=${1:-0} \
  bundle exec rake "knapsack_pro:queue:rspec[--format d --tag xfocus]"

Expected:
I expect the spec/controllers/articles_controller_spec.rb file to be executed because it has xfocus tag. Other test files should not be executed.

Actual:
The spec/features/calculator_spec.rb[1:2:1] test case is executed. This is the issue. It should not be executed.

ArturT added a commit that referenced this pull request Dec 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants