Prevent duplicate output

[mvz]

Summary

Ensure command output is only announced once.

Details

During a run, Command#stop and Command#terminate are called multiple times, among other reasons to ensure all commands are actually terminated at the end of each spec or scenario.

If both general announcers and the announce-on-error functionality is used at the same time, duplicate output may still occur.

This includes changes from #399.

Motivation and Context

This fixes #374.

How Has This Been Tested?

Specs were added by @maxmeyer and myself to ensure stop events are only broadcast once on the event bus. The behavior was confirmed by running some of Aruba's features by hand and examining the entire output produced.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist:

• I've added tests for my code

[olleolleolle]

Communication: I think this is the "Act" step of Arrange-Act-Assert, and that it's not a before-test setup.

Suggestion: Move command.start into the it.

[olleolleolle]

before :each => before (this is the default; and it has been renamed in newer versions of RSpec; :each is now :example)

[mvz]

@olleolleolle I made the changes you suggested, thanks.

[olleolleolle]

@mvz It's a pleasure to learn so much from the Aruba work you do here.

[olleolleolle]

This feature brings clarity to something which can be quite tricky.

[maxmeyer]

@mvz Thanks for tackling this. Personally I prefer this type of specs, which are more "concise" as they use the Rspec syntax to define readable tests: http://www.betterspecs.org/.

[mvz]

@maxmeyer I have two problems with those type of tests:

• Using subject hides the nature of the object under test. Giving it an expressive name allows you to interpret the expectation itself more easily.
• Using an anonymous it block makes the intent of the test less clear: Yes, the method has the expected result, but why do we want it to have that result? Also, some desired outcomes cannot be expressed in RSpec syntax (e.g.,`it 'prevents #stop from notifying the event bus').

[olleolleolle]

@mvz In order to have the cake and eat it, explicitly naming the subject allows

let(:command) do
  described_class.new(
    'true',
    event_bus: event_bus,
    exit_timeout: exit_timeout,
    io_wait_timeout: io_wait_timeout,
    working_directory: working_directory,
    environment: environment,
    main_class: main_class,
    stop_signal: stop_signal,
    startup_wait_time: startup_wait_time
  )
end 

to be

subject(:command) do
  described_class.new(
    'true',
    event_bus: event_bus,
    exit_timeout: exit_timeout,
    io_wait_timeout: io_wait_timeout,
    working_directory: working_directory,
    environment: environment,
    main_class: main_class,
    stop_signal: stop_signal,
    startup_wait_time: startup_wait_time
  )
end 

and, taking it a step further, passing the constructor parameters we're not manipulating in the test as primitive values would reduce "setup".

PS: I find the rspec-its does not do very much for the readability of my testing.

[maxmeyer]

@mvz Sorry, I was not very clear about that. I was a bit in a rush and didn't see, that that use is_expected. I don't like that style either, but try to avoid this.

I try to avoid using strings in the it-method.

RSpec.describe MyClass do
  it 'some description' do
     expect(true).to_be true
  end
end

Instead I try to use something like that:

RSpec.describe MyClass do
  subject(:my_instance) { described_class.new }

  context 'when action was run' do
    # Set state - if required
    before :example do
      my_instance.my_action
    end

    # Assert
     it { expect(my_instance.result).to_be true }
  end
end

I'm open to change the style to something which suits our needs best. But besides that, what I really want to see for our test suite: Use the same style we can commit to everywhere and don't mix styles. Example for the style current used at most places I had a look at: https://github.com/cucumber/aruba/blob/master/spec/aruba/api_spec.rb#L8-L18.

PS: I find the rspec-its does not do very much for the readability of my testing.

@olleolleolle What do you mean with rspec-its

subject(:command) do

Yes, this makes the "sut" explicit.

[mvz]

@maxmeyer agreed on having a consistent style. I'll rewrite the examples and see how that goes.

[olleolleolle]

@maxmeyer Hi!

rspec-its is now a separate RubyGem.

Myron Marston wrote this, before its was removed: Explanation for why its will be removed from rspec-3

[maxmeyer]

@olleolleolle Thanks for clarifying that. Is there any place where we use this kind of style? I searched the repository and didn't find a place where we do this. Did I miss something?

First, I thought http://www.betterspecs.org/ recommends this style, but I can't find anything about rspec-its there either. It's nothing I used before. And reading Myron's writeup makes me thing, this is not a good way to go.

My style of writing RSpec specs was influenced by a talk. I think, it was this one: https://www.youtube.com/watch?v=d2gL6CYaYwQ. Though I don't use the is_expected-syntax for the reasons @mvz wrote down. There's also a later talk on youtube https://www.youtube.com/watch?v=KjENZNjRCWM, but I didn't had a look into it.

The style recommended by this talk helps me to

• concentrate on the way my code behaves in different contexts - using #context "when something is given"
• to DRY up my tests - though this goes "against" readability

But as you both are more into testing than me - my daily job is to keep things running as a sysadmin - I think you've got more experience than me and I'm happy to learn new things. 😄

[maxmeyer]

As discussed: I prefer to use subject(:command) here.

[maxmeyer]

I would place this in a separate #before-block`.

[olleolleolle]

@maxmeyer Now that we're trading video links, here's Searls, talking (very quickly) about testing:

• https://www.youtube.com/watch?v=aiDikM9g4x8
• https://www.youtube.com/watch?v=VD51AkG8EZw

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

[mvz]

@olleolleolle I updated the specs and added one little fix for the InProcess runner. Can you take one final look at this?

[olleolleolle]

Great! Covering those bits is super good.

[olleolleolle]

Let’s get this merged!