Use Ruby 3 in development and add small improvements by ArturT · Pull Request #147 · KnapsackPro/knapsack_pro-ruby

ArturT · 2021-05-24T17:25:58Z

adjust test suite to work with Ruby 3
update development dependencies
remove deprecated codeclimate gem
add support for CirleCI CIRCLE_WORKING_DIRECTORY env var
remove support for detecting project dir for CircleCI 1.0 because it's not longer supported

Starting Mar 15, 2019:

All projects will require a CircleCI 2.0 configuration.
CircleCI 1.0 jobs will be viewable but will no longer run.
https://circleci.com/sunset1-0/eol-policy/

add support for project directory with ~ in path

development tips

If you clone this project in development please remove your local copy of Gemfile.lock (it is ignored in .gitignore) and then run:

rvm get stable
rvm install ruby-3.0.1
bundle install # this will generate a new Gemfile.lock with the latest gems as listed in knapsack_pro.gemspec

…object in Ruby 3 and can't be mocked https://www.mail-archive.com/puppet-bugs@googlegroups.com/msg206907.html

…command requires full path

…require test/unit

…unit while running tests so we can mock method test_unit_autorunner_run

…tcase' to not break RSpec

…s not longer supported Starting Mar 15, 2019: All projects will require a CircleCI 2.0 configuration. CircleCI 1.0 jobs will be viewable but will no longer run. https://circleci.com/sunset1-0/eol-policy/

ArturT · 2021-05-24T18:46:45Z

        end

        def project_dir
-          project_repo_name = ENV['CIRCLE_PROJECT_REPONAME']


Determine project directory with CIRCLE_PROJECT_REPONAME was for CircleCI 1.0.

CircleCI 1.0 is deprecated since 2019 so this code is not needed.

ArturT · 2021-05-24T18:51:42Z

+        dir = KnapsackPro::Config::Env.project_dir
+
+        if dir.to_s.include?('~')
+          File.expand_path(dir)


If the path has ~ sign in ~/repo then it will be expanded to a full path: /home/user/repo.

Thanks to that other parts of the code can work properly, for instance determining git hash and branch name with command like git -C /home/user/repo rev-parse HEAD that is called in this class.

Unfortunately command like git -C ~/repo rev-parse HEAD crashes because it's not full path.

What do you mean by "full path" in the last sentence - do you mean absolute path? Does the problem occur when there is the ~ character in the path specifically, all does it happen with all relative paths? Or maybe it's with all relative paths, but the only ones we encounter contain the tilde?

Basically the reason I'm asking is this: maybe we can always return the absolute path here this way:

def working_dir dir = KnapsackPro::Config::Env.project_dir File.expand_path(dir) end

By full path, I meant absolute path.

Yes, we can do File.expand_path(dir) always. I was worried it won't work when someone provides an invalid path (not existing path on the disk) but even then this method works fine so we could use it always.

ArturT · 2021-05-24T18:57:32Z

+
+      # https://www.rubydoc.info/github/test-unit/test-unit/Test/Unit/AutoRunner#run-class_method
+      def self.test_unit_autorunner_run(force_standalone, default_dir, argv)
+        require 'test/unit'


Method test_unit_autorunner_run has been extracted to not load require 'test/unit' because this breaks the test environment for RSpec and causes random errors.

Not a fan of the require inside a method tbh. :/ Also a bit worried that the test environment dictates how we organize the code containing the actual business logic.

What types of random errors have you encountered? Any idea as to why it worked previously? Was there a reason for the require to previously lie inside of the if statement instead of at the top of the file (where you would typically expect it)? Was this done to avoid other errors maybe?

Have you considered any other solutions to solve the problems you encountered?

knapsack_pro gem supports multiple test runners in a single gem. But we do not list them as dependencies in knapsack_pro.gemspec. They are listed there only as a dev dependency for a testing purpose of knapsack_pro gem.

For instance, if someone is using only RSpec in his project then when he installs knapsack_pro it would be pointless to download all test runners like cucumber, test-unit, etc.

That is why we only require a particular test runner during the runtime of the gem when a method like KnapsackPro::Runners::TestUnitRunner.run is called. Basically, when someone on purpose wants to run knapsack_pro for a given test runner like test-unit then we require require 'test/unit' and we assume the user already has test-unit gem installed in his Gemfile.

In the test environment for knapsack_pro gem we should not load multiple test runners at once because they impact each other. For instance require 'test/unit' breaks RSpec and a lot of weird errors were happening (state of tests was not clean up between test examples) https://app.circleci.com/pipelines/github/KnapsackPro/knapsack_pro-ruby/411/workflows/927f821d-df4f-4213-90d4-cc38c189cfa8/jobs/1398

That is why I extracted test_unit_autorunner_run method to hide require 'test/unit' and not call it in test runtime.

Why it was working previously? I guess I did not refresh my local copy of Gemfile.lock for a few years and when I updated it and new test-unit gem and new rspec gem versions were installed then problem was revealed.

I see, thanks for the extended explanation!

ArturT · 2021-05-24T18:59:21Z

        :allocator
+
+      def self.child_status
+        $?


$? has been extracted to a method child_status because it's not possible anymore to mock $? so let's hide it inside of a method that we can mock.

shadre · 2021-05-25T08:05:50Z

+        dir = KnapsackPro::Config::Env.project_dir
+
+        if dir.to_s.include?('~')
+          File.expand_path(dir)


What do you mean by "full path" in the last sentence - do you mean absolute path? Does the problem occur when there is the ~ character in the path specifically, all does it happen with all relative paths? Or maybe it's with all relative paths, but the only ones we encounter contain the tilde?

Basically the reason I'm asking is this: maybe we can always return the absolute path here this way:

def working_dir dir = KnapsackPro::Config::Env.project_dir File.expand_path(dir) end

shadre · 2021-05-25T08:12:09Z

+
+      # https://www.rubydoc.info/github/test-unit/test-unit/Test/Unit/AutoRunner#run-class_method
+      def self.test_unit_autorunner_run(force_standalone, default_dir, argv)
+        require 'test/unit'


Not a fan of the require inside a method tbh. :/ Also a bit worried that the test environment dictates how we organize the code containing the actual business logic.

What types of random errors have you encountered? Any idea as to why it worked previously? Was there a reason for the require to previously lie inside of the if statement instead of at the top of the file (where you would typically expect it)? Was this done to avoid other errors maybe?

Have you considered any other solutions to solve the problems you encountered?

shadre · 2021-05-25T08:19:55Z

-          project_repo_name = ENV['CIRCLE_PROJECT_REPONAME']
-          "/home/ubuntu/#{project_repo_name}" if project_repo_name
+          working_dir = ENV['CIRCLE_WORKING_DIRECTORY']
+          return working_dir if working_dir


The return keyword is redundant here

…nv var

ArturT · 2021-05-25T10:22:44Z

        if runner.test_files_to_execute_exist?
          adapter_class.verify_bind_method_called

-          require 'test/unit'


require was hide in the run method instead of being at the top of the file to not load test-unit gem during loading knapsack_pro gem.

If someone uses only RSpec then he wouldn't be able to load knapsack_pro gem without having test-unit in the Gemfile. We don't want to force users to install gems they don't use in their projects.

shadre

👍

ArturT added 12 commits May 20, 2021 12:24

Use Ruby 3.0.1 on CI

b22fd3d

Update dev dependencies

e6b483a

Extract child_status in order to mock $? in test because it's freeze …

fbac70c

…object in Ruby 3 and can't be mocked https://www.mail-archive.com/puppet-bugs@googlegroups.com/msg206907.html

Remove codeclimate-test-reporter - it's deprecated

cef051f

use CIRCLECI to verify test executed on CircleCI

8a6a1ca

try to fix git adapter spec on CI

d24df08

determine project dir for CircleCI based on CIRCLE_WORKING_DIRECTORY

cd57c28

if project dir has ~ sign then expand it to full path because git -C …

c9d27f8

…command requires full path

pending tests that are causing problem and affect other tests due to …

87eb7a6

…require test/unit

bring back one before for all tests

81df34a

extract method test_unit_autorunner_run in order to not require test/…

e9f3aad

…unit while running tests so we can mock method test_unit_autorunner_run

use fake class to make tests pass and to avoid require 'test/unit/tes…

5367d14

…tcase' to not break RSpec

ArturT added the enhancement label May 24, 2021

ArturT changed the title ~~Use Ruby 3 in development and small improvements~~ Use Ruby 3 in development and add small improvements May 24, 2021

ArturT added 2 commits May 24, 2021 19:27

Update CHANGELOG.md

10aa849

remove support for detecting project dir for CircleCI 1.0 because it'…

8597b4b

…s not longer supported Starting Mar 15, 2019: All projects will require a CircleCI 2.0 configuration. CircleCI 1.0 jobs will be viewable but will no longer run. https://circleci.com/sunset1-0/eol-policy/

ArturT requested a review from shadre May 24, 2021 18:34

ArturT commented May 24, 2021

View reviewed changes

shadre reviewed May 25, 2021

View reviewed changes

simplify project_dir method to just return CIRCLE_WORKING_DIRECTORY e…

95a77aa

…nv var

ArturT commented May 25, 2021

View reviewed changes

run always File.expand_path(dir)

1a9ff63

shadre approved these changes May 25, 2021

View reviewed changes

ArturT merged commit efd7a75 into master May 25, 2021

ArturT deleted the ruby-3 branch May 25, 2021 12:34

Conversation

ArturT commented May 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

development tips

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shadre left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ArturT commented May 24, 2021 •

edited

Loading