In fact, I got a bit carried away and did a more extensive bit of tidying among the tests. The extra work is over here; there are a bunch of commits because I was working in small low-risk changes, but the summary is:

• Overall goals:
  • make individual tests easier to run
  • clarify use of fixture repo clones
• Steps taken:
  • Eliminate complexity due to regular changes of working directory
  • Make individual test files runnable via ruby -Ilib:tests path/to/test_file.rb
  • Extract constants to refer to fixture locations
  • Reduce unnecessary cloning of certain fixture repos
  • Begin using helper methods to clarify which tests are using which fixture repo

My hope is that once that last point is completed it should be much easier to then work out which fixture repo usage can be eliminated.

I haven't pushed all that to this PR yet because I'm conscious it's a lot to review and spare time is precious, but if any of that sounds of use, I'll be glad to include it.

These changes seem simpler to me, but I haven't made a huge effort to investigate why the loading was the way it was. Are there any ways of running or writing tests that my changes would obstruct?

Not that I know of. If there is, they are unknown to me. These changes look like a step in the right direction to me.

I haven't had the chance to look at your extra work yet.

@urbanautomaton I did want to get your feedback on an alternate idea to run individual tests. Is it a good idea or too hacky?

What if you could simply run a single test right from the rake command line:

$ rake test tests/units/test_archive.rb                             
Loaded suite /Users/james/.asdf/installs/ruby/3.2.0/bin/rake
Started
.
Finished in 0.273002 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
1 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
3.66 tests/s, 54.94 assertions/s
$

or multiple tests:

$ rake test tests/units/test_archive.rb tests/units/test_worktree.rb      
Loaded suite /Users/james/.asdf/installs/ruby/3.2.0/bin/rake
Started
.....
Finished in 0.860063 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
5 tests, 28 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
5.81 tests/s, 32.56 assertions/s
$

or all tests (like normal):

$ rake test
Loaded suite /Users/james/.asdf/installs/ruby/3.2.0/bin/rake
Started
...................................................................................................................................................................
....................
Finished in 27.827281 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
183 tests, 668 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
-------------------------------------------------------------------------------------------------------------------------------------------------------------------
6.58 tests/s, 24.01 assertions/s
$

Here is the rake code that enables it:

desc 'Run Unit Tests'
task :test do
  sh 'git config --global user.email "git@example.com"' if `git config user.email`.empty?
  sh 'git config --global user.name "GitExample"' if `git config user.name`.empty?

  $LOAD_PATH << 'tests'

  test_files = []
  if (test_index = ARGV.index('test'))
    (test_index + 1 .. ARGV.size - 1).each do |i|
      break unless ARGV[i] =~ /^tests\//
      test_files << ARGV[i][6..-1]
    end
  end
  test_files << 'all_tests' if test_files.empty?

  test_files.each { |test_file| require test_file }
end

Thanks for the feedback!

On the rake modification: I know what you mean, it'd be nice to have a single entry point for running tests, and I did consider trying to pass the file args via rake.

The thing that makes me wary of using ARGV in rake tasks though is that it's kinda fighting against the way rake itself handles task arguments (i.e. the official rake way of passing in args would be rake test[path/to/file_1.rb,path/to/file_2.rb]). And in this case, it relies on test being the only task invoked, otherwise the ARGV offsets used would be wrong, so I think it's a bit fragile.

If we did want a single entry point capable of running all tests, one test or a subset of tests, I might be tempted to introduce a very simple bin/test executable to handle those use cases. This could become the documented way of running tests for contributors, and the rake task could remain for CI purposes, just invoking that executable to run all tests. What do you think?

And in this case, it relies on test being the only task invoked, otherwise the ARGV offsets used would be wrong, so I think it's a bit fragile.

The way I have implemented it allows other tasks to be passed on the command line like:

rake build test tests/units/test_archive.rb tests/units/test_profile.rb yardstick

It does work against rake's built in method for passing args, but I find that method to be a bit cumbersome since the square brackets need escaping in zsh.

Does that help?

Also, I reviewed the extra work you did and would love it if those commits were added to this PR. I will squash them all into one commit when I merge to master.

Nice work. This is an area of this gem that has needed cleaning up.

Okay cool, I've pulled in those extra commits. 👍🏻

The way I have implemented it allows other tasks to be passed on the command line[...]

Ah yes, I'd missed that you're scanning forwards for the first ARGV entry that looks like a test file. And agreed that the square-bracket syntax is super cumbersome; I don't think it's a good fit here at all. 👍🏻

TBH my instinct is that we'd still be working around rake's official way of doing things though, which is likely to be a bit fragile and maybe cause developer confusion. So my leaning would remain towards having a separate test runner executable; I've sketched out what that might look like here, see what you think!

Okay cool, I've pulled in those extra commits. 👍🏻

Looks like you broke a test on Windows. Can you take a stab at fixing it? I am guessing since you didn't notice these broken tests, you aren't running windows. Neither do I... so I'd have to guess a solution and then commit the changes to see if the CI tests pass.

TBH my instinct is that we'd still be working around rake's official way of doing things though, which is likely to be a bit fragile and maybe cause developer confusion. So my leaning would remain towards having a separate test runner executable; I've sketched out what that might look like here, see what you think!

Yes, you convinced me. Please add your changes to this PR. If documentation for how tests are run needs to be updated, please do that too. Maybe we should add a bullet in this section of CONTRIBUTING.md that says something about how to run one or more specific test files.

Okay cool - I fixed the windows jobs by backing out one of the smaller changes I made, because those failures were isolated to a single test case I modified (I switched from using a bare repo clone to the 'working' fixture). I'm not sure of the exact cause but there's definitely something suspect going on in the tests that use multiple interlinked fixture repos, and I think that's best left to be investigated later.

Otherwise I've added the test runner executable and documented it in CONTRIBUTING.md, and squashed everything into an omnibus commit with a summary commit message. Ooh, and I added one final cheeky change which was to rename #set_file_paths to #clone_working_repo because that's all that method does now.

I'm definitely stopping there for the moment though; from my end it's ready to go. 😅

This PR:

• Reduced lines of code for code in the tests directory tree by 5% (from 2,700 to 2,555 lines of code)
• Reduced the time it takes to run the entire test suite (on my machine) by 12% (from 27.5 seconds to 24.4 seconds)
• Made it easier to run a single test file or multiple test files so contributors will have less to figure out.
• In many cases, simplified tests so they are easier to understand (and duplicate if needed).

On the whole, this PR removed a lot of tech debt and made this gem easier to change.

Great job Simon (@urbanautomaton )! This is a great contribution!