Consolidate Gemfiles; improve and simplify CI workflow #1002

mattbrictson · 2023-10-20T00:28:26Z

The goal of this PR is to improve the long-term maintainability of vcr by greatly simplifying the CI infrastructure and improving its troubleshooting output.

Improving troubleshooting output

Before, cucumber and rspec commands had their stderr output redirected to a temporary file. The contents of this file were never explicitly printed or archived. If the command failed (as recently happened in #995), maintainers would be flying blind in terms of understanding the failure.

With the changes of this PR, the redirection of stderr still exists, but if a command fails, the stderr output is explicitly printed to the console prior to the CI job exiting. This allows maintainers to see what went wrong in failure scenarios.

Fixes #1000.

Simplifying Gemfiles and CI infrastructure

Before, the CI workflow was organized as a 1:1 conversion of a previous Travis CI setup, which was structured as one big CI script and multiple Gemfiles.

Over the years, the vcr project dropped the appraisal gem in favor of manually managing Gemfile variants. The project also at times tested JRuby and Rubinius, before removing them from CI. These changes and experiments have left a lot of cruft in the testing infrastructure.

This PR performs several cleanup operations:

Removes extra complexity of supporting JRuby and Rubinius. These runtimes were not being tested, so the complexity did not serve a purpose.
Removes separate Gemfiles. The Gemfile.faraday-1.0.0 file, for example, had an overlapping version specification with the one in vcr.gemspec, effectively making it redundant. In the past, CI was configured to run against different major versions of some key dependencies, but that is no longer the case. Now the multiple Gemfiles do not have any benefit. This PR merges them all into a single Gemfile.
Splits the rspec, cucumber, and yard CI steps into parallel jobs. This is something that is easy to do in GitHub Actions that was probably prohibitively difficult in TravisCI. The parallel jobs increase performance and have other benefits.
Moves the stderr output redirection and warning detection logic out of the CI workflow and into a dedicated bash script.

My hope is that with fewer Gemfiles to maintain, and a more familiar GitHub Actions workflow, this PR will make vcr more approachable to maintainers and contributors going forward.

It has been several years since this project was tested under jruby or rbx in CI. This commit removes non-MRI complexity from the Gemfiles that is not needed, since non-MRI platforms are not being tested.

The base Gemfile already restricted faraday to versions `< 2`, so the extra `Gemfile.faraday-1.0.0` was redundant. This commit removes that separate Gemfile and merges its dependencies into the main Gemfile.

mattbrictson · 2023-10-20T00:50:05Z

.github/workflows/test.yml

@@ -1,69 +1,56 @@
-name: vcr ci
+name: CI


🗒️ I shortened this name for more concise output.

mattbrictson · 2023-10-20T00:50:33Z

.github/workflows/test.yml


 on:
  push:
-    branches: [ master ]
+    branches: [master]


🗒️ I ran this file through Prettier to apply standard YAML formatting and indentation.

mattbrictson · 2023-10-20T00:52:34Z

.github/workflows/test.yml

  pull_request:
    types: [opened, synchronize, reopened]

 jobs:
-  test:
+  cucumber:


🗒️ I separated rspec, cucumber, yard steps into their own jobs for the following reasons:

CI can complete faster, since jobs run in parallel.

Developers see more precise status about which parts of CI are passing/failing.

The yard step can be optimized to run with less setup, because it doesn't need the apt-get steps.

mattbrictson · 2023-10-20T00:54:27Z

.github/workflows/test.yml

-
-    - name: faraday-1.0.0
-      run: |
-        bundle install


🗒️ The old CI setup had multiple invocations of bundle install with different Gemfiles. There were not any important differences between the Gemfiles, so I removed these redundant installations and consolidated dependencies to a single Gemfile.

mattbrictson · 2023-10-20T00:54:59Z

.github/workflows/test.yml

-
-    - name: check warnings
-      run: |
-        vcr_warnings="$(grep -F "$PWD" "${ALL_WARNINGS}" | grep "warning: " | grep -v "${PWD}/vendor/bundle" | sort | uniq -c | tee /dev/stderr | wc -l)"


🗒️ I moved the warning detection code into a new dedicated bash script, script/fail_if_warnings.

mattbrictson · 2023-10-20T00:56:49Z

Gemfile

 gemspec

-gem 'jruby-openssl', :platforms => :jruby


🗒️ CI has not tested against JRuby in quite a long time. There was extra complexity for non-MRI runtimes in the Gemfiles that was not providing any benefit, because CI was never being run on anything besides MRI. I decided to remove this complexity and write the Gemfile assuming MRI.

mattbrictson · 2023-10-20T00:57:39Z

Gemfile

-  gem "em-http-request"
-  gem "curb", "~> 1.0.1"
-end
+gem "aruba", "~> 0.14.14"


🗒️ I moved all development dependencies out of vcr.gemspec and into the main Gemfile. The standard project structure recommended by bundle init these days advocates for organizing dev dependencies in this way.

mattbrictson · 2023-10-20T00:59:11Z

script/fail_if_warnings

+warnings=/tmp/vcr_warnings.out
+touch "${warnings}"
+
+RUBYOPT=-w bundle exec "$@" 2> "${warnings}" || { cat "${warnings}" > /dev/stderr; exit 1; }


🗒️ This pipes stderr from the Ruby process to a warnings file. If the Ruby process fails for whatever reason, the warnings file is printed to the console and the script exits. This fixes a problem with the old CI script where stderr was being swallowed if the Ruby process crashed. Now we can easily troubleshoot when this happens.

mattbrictson · 2023-10-20T00:59:59Z

script/fail_if_warnings

+warnings_count="$(grep -F "$PWD" "${warnings}" | grep "warning: " | grep -v "${PWD}/vendor/bundle" | sort | uniq -c | tee /dev/stderr | wc -l)"
+if [[ "${warnings_count}" -gt 0 ]]; then echo "FAILED: test suite doesn't tolerate warnings"; exit 1; fi


🗒️ This code is unmodified from its previous version. I simply moved it out of the Actions workflow into this script and renamed some variables for clarity.

mattbrictson · 2023-10-20T01:00:18Z

vcr.gemspec

@@ -20,25 +20,4 @@ Gem::Specification.new do |spec|
  spec.require_paths = ["lib"]

  spec.required_ruby_version = ">= 2.7"
-
-  spec.add_development_dependency "bundler", "~> 2.0"


🗒️ These dependencies are now declared in the Gemfile.

mattbrictson · 2023-10-20T01:25:46Z

.github/workflows/test.yml

+  doc-coverage:
+    name: "Doc coverage"
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v4
+      - uses: ruby/setup-ruby@v1
+        with:
+          ruby-version: "ruby"


🗒️ I assumed that running the yard coverage check against a matrix of multiple versions of Ruby was overkill. I changed it to run on the latest version of Ruby only.

mattbrictson · 2023-10-20T01:30:15Z

CONTRIBUTING.md

-If you get an error while running `bundle install`, it may be one of the "extras" gems which are not required for development. Try installing it without these gems.
-
-```console
-bundle install --without extras


🗒️ There is no extras group in the Gemfile, so this documentation was misleading. I removed it to prevent confusion.

olleolleolle

Thank you, @mattbrictson! I agree with all the changes, and thanks also for all the context-preserving notes in the Description and comments of this PR.

mattbrictson added 7 commits October 19, 2023 16:47

Remove jruby/rbx complexity from Gemfiles

4d0819b

It has been several years since this project was tested under jruby or rbx in CI. This commit removes non-MRI complexity from the Gemfiles that is not needed, since non-MRI platforms are not being tested.

Remove separate faraday Gemfile

f02181f

The base Gemfile already restricted faraday to versions `< 2`, so the extra `Gemfile.faraday-1.0.0` was redundant. This commit removes that separate Gemfile and merges its dependencies into the main Gemfile.

Consolidate all dev dependencies into a single Gemfile

73d1a8a

Simplify CI by introducing fail_if_warnings script

21c5ae5

Capture stderr and show it only if command fails

5d49699

Split CI workflow into separate jobs

39435bf

Fix incorrect operator precedence

197db17

mattbrictson commented Oct 20, 2023

View reviewed changes

Remove misleading reference to 'extras', which doesn't exist

19aeebc

mattbrictson commented Oct 20, 2023

View reviewed changes

olleolleolle approved these changes Oct 20, 2023

View reviewed changes

olleolleolle merged commit bf8e1b0 into vcr:master Oct 20, 2023

mattbrictson deleted the chores/simplify-gems-and-ci branch October 20, 2023 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidate Gemfiles; improve and simplify CI workflow #1002

Consolidate Gemfiles; improve and simplify CI workflow #1002

mattbrictson commented Oct 20, 2023 •

edited

Loading

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

mattbrictson Oct 20, 2023

olleolleolle left a comment

		warnings_count="$(grep -F "$PWD" "${warnings}" \| grep "warning: " \| grep -v "${PWD}/vendor/bundle" \| sort \| uniq -c \| tee /dev/stderr \| wc -l)"
		if [[ "${warnings_count}" -gt 0 ]]; then echo "FAILED: test suite doesn't tolerate warnings"; exit 1; fi

Consolidate Gemfiles; improve and simplify CI workflow #1002

Consolidate Gemfiles; improve and simplify CI workflow #1002

Conversation

mattbrictson commented Oct 20, 2023 • edited Loading

Improving troubleshooting output

Simplifying Gemfiles and CI infrastructure

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleolleolle left a comment

Choose a reason for hiding this comment

mattbrictson commented Oct 20, 2023 •

edited

Loading