Show output when command fails with "fail_on_error"

[njam]

Happy New Year!

When testing some long-running flaky commands it can be quite difficult to understand what is causing a command to fail.
I think it could be useful to show the command's output if it failed unexpectedly.

This might be completely the wrong way of accomplishing that.
Wdyt in general of the idea?

[ghost]

Sounds a like a very good idea. I had the same situation myself more than once. Let me think about the implementation a bit.

[ghost]

Just some of my thoughts, no need to implement this. Will come back during the next couple of days and give you proper feedback.

• Does this need to be configurable? No all users might want this kind of behaviour. This will add more complexity
• What about activate_*-methods in RSpec and @activate_-tags in cucumber? Is it a problem with a configuration for this and the activate things?
• It's better to have singular feature tests. It's better to split up the feature test in two.
• Change some of the command matchers to output more information if the configuration says output stdout/stderr

[njam]

I agree with all your points. Since I'm not familiar with the "announcer" concept and its configurability I can't say for sure what's the best way.

I understand the "activate" directives can only be used to enable an announcer channel, but never to disable one. So they should not be in conflict with this, where we also only enable the channels?

A configuration for this sounds reasonable. I think it would be nice if this feature was enabled by default, as it's quite helpful, especially for beginners. But not sure if that would be considered a "breaking" change, and what constraints you have in that regard?

[maxmeyer]

But not sure if that would be considered a "breaking" change, and what constraints you have in that regard?

I would just follow my gut feeling here. I would consider this a breaking change. I also added another idea I had to my previous post. Maybe I makes more sense to just change the matchers to output more information - not the full stderr/stdout but say 10, 25, 50 or 100 lines. That feels a bit more appropriate than re-use the announcer for this. But I'm stil not sure, what's the best way. I will come back during the next week.

[ghost]

Ok. I think I've an idea about how we can solve this in a quite general manner.
Hope this makes sense to you. Just give me a ping, if you've got problems with this suggestions or implementing them.

• First of all I think we're on a very good way with that what you've already coded

With those suggestions below I hope to make the code look the same like the existing one

• Use the announcer to output information
• Use an option to output certain announcers on command failure in #run_simple,
  :activate_announcer_on_command_failure. This needs to be added to
  lib/aruba/config.rb and tests for that need to be added to
  features/configuration/activate_announcer_on_command_failure.feature.
  Examples for tests can be found in other feature files in the
  configuration-directory. This option is an array of certain announcers,
  maybe stdout, stderr, command_filesystem_status, exit_status. The values
  the users can choose should be restricted by using an enum contract
  Aruba::Contracts::Enum. Default value should be []. Maybe you need to use
  the Maybe-contract here (https://github.com/egonSchiele/contracts.ruby).
  Please do me a favor and write those test like "documentation".
• Add something like announcer.activate config.activate_announcer_on_command_failure to the block you already
  defined in your code in #run_simple. This will work when I merged Fixed announcers #338.
  This PR should make your life a lot easier. Using this, we can make sure,
  that the user can decide what announcements she's interested in.
• Please don't extend existing feature test, but adding new ones explicitly
  explaining that functionality. I tried to explain only one feature per
  example - if possible.
• Please rebase. I refactored and fixed the announcer

[ghost]

@njam Are you still interested to make that addition to aruba? As I like your idea I would like to see it implemented and released with 0.13.0.

[njam]

@dg-ratiodata VERY interested! Also VERY sorry for the delay. I will try to to work on it this weekend!

[maxmeyer]

Great!

[njam]

Thank you very much for the explanations! I guess it takes you probably more time to explain as if you would just implement it. Much appreciated!

I pushed a new commit.

I wasn't sure about the contract for :activate_announcer_on_command_failure. I understand you wanted me to limit the possible channels that one can configure with an "Enum" contract. When I was looking at announcer.rb I thought though that these channels are not pre-defined, rather the user can use his own channel names. So shouldn't we allow any channels to be activated then?

Regarding the feature test - I'm now activating :stdout and :stderr and checking that they show up in the output. Does it make sense?

[ghost]

Looks very good!

So shouldn't we allow any channels to be activated then?

You're right with that. Allow any channel.

Thank you very much for the explanations! I guess it takes you probably more time to explain as if you would just implement it. Much appreciated!

Thanks.

Two last things and then we're good to go.

1. There's a failure in the build. I think you need to change the expectation in line 4488 to make ruby 1.8.7 happy.

2. Can you please add another example to the cucumber run steps documentation? Just copy one of the other examples and add another step twith appends/creates step_definitions.rb with the following/similar content and then check if the output of the inner cucumber run contains the output of the failed command.

   Before do
     aruba.config.activate_announcer_on_command_failure = [...]
   end

[ghost]

Mmmh... I would use a Before-hook here.

The configure-syntax should go to features/support/aruba.rb. That would be another example. I thought about adding such an example, too. But that would mean, that we need to do that for all other configuration options as well. I don't think it's worth the effort. Just change it to Before. That should be enough.

There's also a general section about configuration that can be found here.

[njam]

Oh! 💡
More like this?

[njam]

Now I'm appending to features/support/env.rb. Or it should go to step_definitions.rb?

[ghost]

Does this work on ruby 1.8.7? I think this [:foo, :bar]caused a failing test in one of the previous builds.
Forget about this comment, I didn't see the .inspect.

[njam]

smuggled it in ;)

[ghost]

Ah, and please rebase. I pushed some commits this morning to fix an issue.

[njam]

@dg-ratiodata wdyt?

[maxmeyer]

I think we're good to go! Merged. Thanks a lot @njam for your contribution! Great work.

[ghost]

FYI: We're now using your code in aruba's test suite! And well, it helped me to understand an error! 👍

[njam]

Cool cool, looking forward to use it as well. Thanks again for the assistance!

[ghost]

I released your changes with aruba 0.13.0 a couple of minutes ago.