Avoid loading all tasks when first loading the CLI

[davidstosik]

Loading all tasks is only necessary when displaying the long description for the perform task. Any other operation from the CLI (such as displaying the general help message or performing a task) should not load all tasks.

[adrianna-chang-shopify]

Thank you! ❤️

[etiennebarrie]

This doesn't seem necessary.

[davidstosik]

Nice catch, I first tried stubbing IO.console_size but that didn't seem to work. Will remove.

[etiennebarrie]

We can get rid of this if we remove the long line which is wrapped from the expected output.

[davidstosik]

I don't think that works.
It looks like Thor unwraps the message then wraps it again to the current terminal width.
I tried with this code:

    commands["perform"].define_singleton_method(:long_description) do
      <<~LONGDESC
        `maintenance_tasks perform` will run the
        Maintenance Task specified by
        the [TASK NAME] argument.

        Available Tasks:

        #{Task.available_tasks.map(&:name).sort.join("\n\n")}
      LONGDESC
    end

and the test failure looks like this:

Failure:
MaintenanceTasks::CLITest#test_`help_perform`_loads_all_tasks_and_displays_them [test/lib/maintenance_tasks/cli_test.rb:106]:
In stdout.
Expected /\ \ `maintenance_tasks\ perform`\ will\ run\ the\ Maintenance\ Task\ specified\ by\ the\ \[TASK\ NAME\]\ argument\.\n\n\ \ Available\ Tasks:\n\n\ \ Task1\n\n\ \ Task2\n/ to match "Usage:\n  cli_test.rb perform [TASK NAME]\n\nOptions:\n  [--csv=CSV]              # Supply a CSV file to be processed by a CSV Task, --csv path/to/csv/file.csv\n  [--arguments=key:value]  # Supply arguments for a Task that accepts parameters as a set of <key>:<value> pairs.\n\nDescription:\n  `maintenance_tasks perform` will run the Maintenance Task specified by the [TASK\n  NAME] argument.\n\n  Available Tasks:\n\n  Task1\n\n  Task2\n".

The actual output looked like this around the checked portion:

Description:
  `maintenance_tasks perform` will run the Maintenance Task specified by the [TASK
  NAME] argument.

  Available Tasks:

If I run the test in a wide terminal, it works. If I run it in a small terminal, it fails. 😅

I could however add more line returns in the help message, like so:

    commands["perform"].define_singleton_method(:long_description) do
      <<~LONGDESC
        `maintenance_tasks perform` will run the

        Maintenance Task specified by

        the [TASK NAME] argument.

        Available Tasks:

        #{Task.available_tasks.map(&:name).sort.join("\n\n")}
      LONGDESC
    end

Thor would then keep the lines as they are, instead of rewrapping.

[davidstosik]

Disabling wrapping for long descriptions was added to Thor only 3 months ago, merged (May 15) after the last release 1.2.2 (May 11): rails/thor#739

If we don't want to make our dependency on Thor stricter, then we need to expect that sequences of lines in a long description will be rewrapped to fit in the current terminal's width.

Another way to work around the issue would be to enforce a terminal width (80?) for the whole test suite.

[davidstosik]

To summarize:

• Thor now supports disabling wrapping of the long description (🌧️ long_desc: new option to disable wrapping rails/thor#739), but relying on that would mean constraining our dependency on Thor.

• The output produced by the CLI when running in tests will depend on the width of the terminal the tests are run in. If I run tests in a 100-char terminal, then the long description will be wrapped at 100 characters. If CI runs the same tests in a 60-char terminal, then the long description will be wrapped accordingly, at 60 characters. Writing a test that works in both contexts requires handling that discrepancy, and stubbing the terminal width is one way to do so.

• Another way to do so would be to avoid Thor's rewrapping, but the only way to do so would be to:

  1. Keep lines short enough that we're confident they wouldn't be rewrapped individually in a developer's terminal or in CI. (40 characters? 60?)
  2. And also add an empty line between each line of text to prevent Thor from rewrapping the whole paragraph.

  I don't think that's a good workaround.

I'll merge as-is, but feel free to let me know if you'd like me to implement a better workaround.

[etiennebarrie]

Yeah I meant that in the assertion, we could just not care about the long line, and all the other lines won't get wrapped anyway. The idea was to tweak the test to avoid having the test know about the implementation details of Thor, while staying useful, and not change the code.
But in my testing I was limited by Terminal.app 20-row minimum, using iTerm, reducing the size to something above 10 (because Thor has a minimum of 10) but below 19 rows (" Available Tasks:".size # => 18) we also get wrapping on the "Available Tasks:" line, which is a bit annoying. I guess we can still remove both and have an acceptable test without leaking Thor's implementation:

diff --git i/test/lib/maintenance_tasks/cli_test.rb w/test/lib/maintenance_tasks/cli_test.rb
index 681a06a..ebf6a80 100644
--- i/test/lib/maintenance_tasks/cli_test.rb
+++ w/test/lib/maintenance_tasks/cli_test.rb
@@ -92,17 +92,12 @@ class CLITest < ActiveSupport::TestCase
         .returns([stub(name: "Task1"), stub(name: "Task2")])
 
       expected_output = <<~OUTPUT.indent(2)
-        `maintenance_tasks perform` will run the Maintenance Task specified by the [TASK NAME] argument.
-
-        Available Tasks:
-
         Task1
 
         Task2
       OUTPUT
 
       assert_output(Regexp.union(expected_output)) do
-        Thor::Base.shell.any_instance.stubs(:terminal_width).returns(200)
         CLI.start(["help", "perform"])
       end
     end

[davidstosik]

Ohh I see, you meant to completely remove expectations on that part of the output, to just confirm that a list of all tasks is in the output. That makes sense too I think. Let me know if you'd like me to open a PR with that change. 🙏🏻

[davidstosik]

avoid having the test know about the implementation details of Thor

I agree, that would have been better. Unfortunately, I couldn't figure out how to stub something more generic at the terminal level. I thought stubbing IO.console_size would work because of this line in Rails, but it does not, because Thor forks to shell commands (stty, tput) to get the terminal width. (I wonder if Thor should switch to using IO.console_size. 🤔)

[etiennebarrie]

Instead of Regexp.escape then constructing a Regexp with the resulting String, I usually use Regexp.union with one parameter, maybe it's less intention revealing though.

[davidstosik]

I like that! 👍🏻