Avoid needing a shell for rspec

[liath]

Story

We're migrating some services to distroless and generally prefer to run tests in the final built image but Knapsack requires a shell for this one method. I've monkeypatched my way around it in my repos but thought it's a simple change that others may benefit from so here we are.

Description

Rather than pass these envvars in the command, which forces system() to use a shell, we pass them as a hash in the first param. Now we can run knapsack in a container that lacks a shell.

Changes

TODO: changes introduced by this PR

Checklist reminder

• You added the changes to the UNRELEASED section of the CHANGELOG.md, including the needed bump (ie, patch, minor, major)
• You follow the architecture outlined below for RSpec in Queue Mode, which is a work in progress (feel free to propose changes):
  • Pure: lib/knapsack_pro/pure/queue/rspec_pure.rb contains pure functions that are unit tested.
  • Extension: lib/knapsack_pro/extensions/rspec_extension.rb encapsulates calls to RSpec internals and is integration and e2e tested.
  • Runner: lib/knapsack_pro/runners/queue/rspec_runner.rb invokes the pure code and the extension to produce side effects, which are integration and e2e tested.

[ArturT]

@liath Could you elaborate on the issue that you were experiencing before this change?

Something I'd like to ensure is that we are isolating the command Kernel.system({ 'RACK_ENV' => 'test', 'RAILS_ENV' => 'test' }, cmd), and it won't pollute the memory of the main process that invoked it. Is that still the case?

Thanks.

[ArturT]

@liath, could you create a new empty commit so that we can trigger the CI pipeline and verify if the tests are passing? Additional tests run on CI. Thanks.

git commit --allow-empty -m "empty commit"

[liath]

@ArturT Here's the smallest slice of our setup I could make to reproduce the issue, it's still kinda large, sorry 😓

Save this as Dockerfile:

FROM cgr.dev/chainguard/ruby:latest-dev AS builder

WORKDIR /build
RUN <<RUN
cat <<GEMFILE > Gemfile
source "https://rubygems.org"
gem "knapsack_pro"
gem "logger"
gem "rspec"
GEMFILE

gem install bundler
bundle config set --local path vendor/bundle
bundle install
RUN

RUN <<RUN
cat <<RAKEFILE > Rakefile
require 'knapsack_pro'
KnapsackPro.load_tasks if defined?(KnapsackPro)
RAKEFILE

mkdir bin
cat <<CI > bin/ci
ENV['PATH'] =+ "#{Gem.user_dir}/bin"
require 'bundler/setup'
require 'knapsack_pro'

KnapsackPro::Runners::Queue::RSpecRunner.run('--format d')
CI

mkdir spec
cat <<SPEC_HELPER > spec/spec_helper.rb
require 'knapsack_pro'
KnapsackPro::Adapters::RSpecAdapter.bind
SPEC_HELPER

cat <<TEST_SPEC > spec/test_spec.rb
require 'spec_helper'
RSpec.describe Object do
  it 'meows' do
    expect('meow').to eq('meow')
  end
end
TEST_SPEC
RUN

FROM cgr.dev/chainguard/ruby:latest

WORKDIR /app
COPY --from=builder /home/nonroot/.local /home/nonroot/.local
COPY --from=builder /build /app
CMD ["bin/ci"]

ENV KNAPSACK_PRO_BRANCH=main
ENV KNAPSACK_PRO_CI_NODE_BUILD_ID=0
ENV KNAPSACK_PRO_CI_NODE_INDEX=0
ENV KNAPSACK_PRO_CI_NODE_TOTAL=2
ENV KNAPSACK_PRO_COMMIT_HASH=0000
ENV KNAPSACK_PRO_FIXED_QUEUE_SPLIT=true
ENV KNAPSACK_PRO_RSPEC_SPLIT_BY_TEST_EXAMPLES=true
ENV KNAPSACK_PRO_SLOW_TEST_FILE_PATTERN="{spec/*}"
ENV KNAPSACK_PRO_SLOW_TEST_FILE_THRESHOLD=1

Then run:

# you'll need to export the creds for Knapsack
docker build -t test . && \
  docker run --rm -it -e KNAPSACK_PRO_TEST_SUITE_TOKEN_RSPEC -e KNAPSACK_PRO_SALT -e CI=true test

You'll get a fairly opaque stack trace like:

 Exception backtrace: /app/vendor/bundle/ruby/3.4.0/gems/knapsack_pro-8.1.1/lib/knapsack_pro/adapters/rspec_adapter.rb:34:in 'KnapsackPro::Adapters::RSpecAdapter.test_file_cases_for'
...

We can monkeypatch in my changes from this PR by inserting the following into the bin/ci script before the last line:

KnapsackPro::Adapters::RSpecAdapter.class_eval do
  def self.test_file_cases_for(slow_test_files)
    cmd = [
      KnapsackPro::Config::Env.rspec_test_example_detector_prefix,
      'rake knapsack_pro:rspec_test_example_detector'
    ].join(' ')
    unless Kernel.system({ 'RACK_ENV'=>'test', 'RAILS_ENV'=>'test' }, cmd)
      raise "Could not generate JSON report for RSpec. Rake task failed when running #{cmd}"
    end

    # read the JSON report
    KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector.new.test_file_example_paths
  end
end

And the test will now pass.

As for polluting memory space, resource cost should actually be lower as we now aren't spawning a child shell to run the same task. I don't consider myself an expert in ruby however so perhaps there's something non-obvious about system that I'm unaware of?

[ArturT]

@liath Thank you. I'll bring this to our team.

[liath]

This is incomplete fwiw but was the minimum to get us moving. There's at least a few places where we'd need to move shell redirection syntax to the redirection params. Eg:

# before:
`git log --format="%aN <%aE>" -1 2>/dev/null`
# after:
system('git log --format="%aN <%aE>" -1', err: File::NULL)

[ArturT]

@liath Thanks, that's a good point.

I’d like to propose a few alternatives:

1. We could release just the change you committed in this PR.
2. If you’d like to contribute other changes (e.g., git), we’d be happy to review and release them too.
3. We could look into the changes required for 2. ourselves, but we may need a bit of time.

What do you think?

[liath]

2. If you’d like to contribute other changes (e.g., git), we’d be happy to review and release them too.

I think I have all of the bases covered for this now. The syntax when the command's output is desired is... verbose, especially around piping: https://github.com/KnapsackPro/knapsack_pro-ruby/pull/296/files#diff-b2689facc4577d4d432168b99958c787d8e07fe8df6891a8dbd2160b9f961751R57

But it should all work as before so the e2e tests failing is weird, I don't think I will have changed error handling anywhere that would cause this. The places I switched backticks to system will hide errors but that's limited to the git adapter. :\

[ArturT]

@liath Thank you for your contribution. We're going to review this. It has been added to our backlog.

Regarding failing e2e tests, CI secrets are not available for builds from external contributors, which makes the tests fail. I'll need to check this.

May we push changes on top of your commits to adjust things and ensure CI works?
Or we will create a separate PR based on your changes when we review this PR.

Thanks again for your help!

[liath]

Regarding failing e2e tests, CI secrets are not available for builds from external contributors

@ArturT ah, of course lol. I just assumed it was something I broke somehow :P

May we push changes on top of your commits to adjust things and ensure CI works?

Yeah, no worries!

[ArturT]

@liath We've released a new gem version 8.2.0 that incorporates your changes from this PR. I had to create a separate PR to run tests on CI.

Thank you for your contribution!

[3v0k4]

Hey @liath.

We found some issues (#301) so we reverted these changes (#302).

We'll go back to this and try to debug what happened, but I'm afraid you'll have to monkey-patch for the time being if you want to upgrade from 8.2.0.

Sorry for this!