PR for Refactor shutdown sequence to use plugin.stop

[guyboertje]

Shutdown semantics: Add stop method and shared_example
Add general tests (none before)
slight refactor
tests pass

Notes for Reviewers:

• This is a mixed PR, it contains code changes for the fix and general refactoring, because this plugin is very distant from core, the codebase has changed little since 2012 and there were no tests.
• The requires are not lazy loaded.
• Following our change in understanding about ivars and concurrency, I moved the ivar creation into the initialize method.
• I feared that when multiple rooms were specified, all but the last MUC client might be GC'd so I stored them in an array.
• I made the client and muc_clients available as attr readers so the tests could mimic the block call action when receiving messages.
• The tests use Mock classes that provide the same API as the Jabber Client and MUCSimpleClient. Normally a test purist would say this is wrong because the API could change etc. I considered this and figured that as the xmpp4r gem development has essentially stopped the risk was v low.
• The Mock classes are inline and not in a separate file so others can read them easily. I suspect, beyond bug fixes in this PR, this plugin will probably not see any more changes for some time.

Closes #3

[talevy]

should we move this to the top or bottom of the file for easy finding?

[guyboertje]

My thinking: as its used in methods from register downwards it should be above def register.

We don't really enforce the classic ruby file layout and for plugins I think the config DSL should standout at the top. My 2c.

[jsvd]

super is no longer needed

[purbon]

I would move this mocks to an spec_helper.rb file or other kind of support helper concept.

[guyboertje]

I considered this and made a point about it in the Reviewers Notes above. What rule of thumb should we apply here? On the one hand it is tidy to move the mocks to their own file and on the other hand, moving them will make it less easy to read and follow the whole spec suite in one go.

I cannot find any solid guidance in the Internet about extract or not wrt helpers. Even the Rspec style guide is silent on this.

The only RSpec convention is to put them in /support and they get auto-loaded but then the same people that don't know what described_class means will not know about this RSpec convention either.

[purbon]

Hi,
as there no official logstash style guide about it, it end up very personal. But in my experience, keeping the test without the definition of mocks and other support classes makes sense as code is simpler and easier to understand then.

I would strongly recommend you to do it as will keep things easy to understand and manage in the long term. But again this is my opinion, i tend to like everything in their place, and for this example mocks belong to their definition files and test belong here, mixing them confuses the read, and somehow makes stuff tied up.

On the other side, even if not official this is a pattern I'm trying to enforce in the hole code base, so it would be nice to have mocks defined in helper files that then gets used in the examples.

Again, is the same example as having smoller methods as possible, we're humans and to understand big pieces of code it gets error prone.

[purbon]

LGTM, keep in mind to squash the commits before merging 👍

[elasticsearch-bot]

Merged sucessfully into master!