Allow tests to run outside of the gem context with the manpage help topic

[aerostitch]

I'm packaging the latest asciidoctor version for Debian and right now it fails because of the test reading the man page form a non-standard location (which fails without trying the standard location).
This PR would check in the standard man page location (so it would not fail once the package is installed via distribution package installation).

The 1st case is when the installation has been done in a non-gzip way (can happen when you are building the package) and the other one is once you've installed the package.

[mojavelinux]

Thank you for this PR.

The purpose of the manpage help topic is to provide access to the manpage when the package isn't installed system-wide. That's why it looks inside the installed gem and not on the system.

If we do decide to extend support to the system-wide manpage, I don't think we should crawl upwards in the filesystem to find the man page. That feels fragile. What I'd rather is use the man command to find the man page man -w asciidoctor.

We don't need to unzip the result as that's already handled when piping it to man:

asciidoctor -h manpage | man -l -

(and it's pretty useless without piping it to man because who is going to read the manpage source?)

[aerostitch]

One of the main issue is that because there are unit tests on this particular functionality.
Without that PR we're unable to have the tests running properly unless you have another way to do.

[mojavelinux]

I'm not sure I understand. Why are you running the tests outside of the context of the gem? They are not designed with that scenario in mind.

...having said that, I'm fine with adding an exception in the test suite if this the only case that's failing. I don't want to change the implementation to accommodate the test. The implementation should be changed to reflect how it actually gets used (hence why I'm suggesting using man to find the location). That's what we should work towards.

[aerostitch]

I'm packaging asciidoctor for Debian so yes, it's running outside of the gem context.

I hear you when you say not to change the implementation to accommodate the context, but the current search for the man page is very gem-specific and doesn't take in account the reality of OS packaging.

I didn't want to make another debian-based-distros-specific patch to skip yet another test instead of making the search path a little more flexible.
Do you recommend to skip the test if not in a gem context instead?

[mojavelinux]

I'm packaging asciidoctor for Debian so yes, it's running outside of the gem context.

This is a new requirement, so we have to address it as such. I was not aware the test suite was being run outside of the project itself. But I'm happy to make adjustments so it can be. But I'll also have to keep that in mind when writing tests.

but the current search for the man page is very gem-specific and doesn't take in account the reality of OS packaging.

You're accurately describing why this feature was implemented. It was designed to fill the gap when the gem is not installed system-wide, meaning when man asciidoctor is not available. That's why the message say "try man asciidoctor".

But you've made me realize that there's no reason it can't support both cases. After all, why not.

I didn't want to make another debian-based-distros-specific patch to skip yet another test instead of making the search path a little more flexible.

I'm completely open to making the search path more flexible. But if we are going to use a system path, I want to resolve it the system path way, which is using man -w. I just don't want to do ../../share/man as that's extremely system specific. And if we use man -w to find it, the test should pass (unless there's something else I'm missing).

[mojavelinux]

In order to work on all Ruby implementations, we'll need to use the following to invoke the system command:

manpage_path = Open3.popen3('man -w asciidoctor') {|_, out| out.read.chomp }

[aerostitch]

So there are several issues about that:

• the first elsif was designed to run during the dh_auto_install too which installs the future file structure inside a directory that looks like /<<PKGBUILDDIR>>/debian/asciidoctor/usr/...
• the 2nd elsif was meant to also work in minimal environments where the man command is not available like in the CI we have in Debian: https://ci.debian.net/

That said, I think I do have a solution that would be more aligned with your 1st remark and closer to what it was designed for in the first place: conditional testing based on the presence of an environment variable.
Let's say the environment variable we use would be named ASCIIDOCTOR_MANPAGE_ABSENT_FROM_TEST, we would run a test that ensures that the exit code is 1 and the error message is present if the environment variable is present (and skip the other case if present).
And skip this new test if absent like so:

  test 'should dump man page and return error code 0 when help topic is manpage' do
    skip 'this test should run only in a gem install context' if ENV['ASCIIDOCTOR_MANPAGE_ABSENT_FROM_TEST']
    exitval, output = redirect_streams do |out, _|
      [Asciidoctor::Cli::Options.parse!(%w(-h manpage)), out.string]
    end
    assert_equal 0, exitval
    assert_includes output, 'Manual: Asciidoctor Manual'
    assert_includes output, '.TH "ASCIIDOCTOR"'
  end

  test 'should print a message and return error code 1 when help topic is manpage' do
    skip 'this test should run outside of the gem context' unless ENV['ASCIIDOCTOR_MANPAGE_ABSENT_FROM_TEST']
    redirect_streams do |out, stderr|
      exitval = Asciidoctor::Cli::Options.parse!(%w(-h manpage))
      assert_equal 1, exitval
      assert_equal 'asciidoctor: FAILED: man page not found; try `man asciidoctor`', stderr.string.chomp
    end
  end

This way, all the distributions packaging asciidoctor would have to do is set this environment variable to anything to make it work.

And if you want to test this specific use case in your CI you just have to remove the man page when you run the test and set the environment variable and you'd be all set.

Would that be an acceptable/flexible enough solution @mojavelinux ?

Thanks
Joseph

[aerostitch]

I updated this PR with the new proposed code.

[mojavelinux]

Stay tuned. I've been busy getting some other things merged. I'll follow-up soon.

[mojavelinux]

the first elsif was designed to run during the dh_auto_install too which installs the future file structure inside a directory that looks like /<>/debian/asciidoctor/usr/...

Now I understand the use case. Thank you for clarifying.

I still don't want to either skip the test or hack the behavior. I just want to fix the behavior. But I do like where you are going with an environment variable. Instead of using an environment variable to skip the test, let's set the environment variable to provide the location of the manpage (ASCIIDOCTOR_MANPAGE_PATH). That way, this even becomes useful at runtime (such as the case when the man command is not available to resolve it).

I will submit an update to the PR for you to review.

[mojavelinux]

I've made the update. Let me know how this works out for you. You just need to set the ASCIIDOCTOR_MANPAGE_PATH environment variable to the location of the manpage file when building the package.

If you have another suggestion for the variable name, just let me know.

I also decided to go ahead and restore the logic to uncompress the stream. I can see reasons why this is valuable because otherwise reading the file as text could mess up the encoding. However, I switched to the block form so that the file is properly closed.

[aerostitch]

Awesome, thanks, the change looks really good. The variable name is great and explicit.
Looks good to me! :)

[mojavelinux]

Fantastic! Thanks for following up.