Allow nil parent for tests

[moolitayer]

Like other managers with parents, we must allow_nil to pass
manageiq-ui-classic's tree_node_spec.rb creation with out a parent obeject (which should never happen in real life). Reproduce using ManageIQ::Providers::Openshift::MonitoringManager.new (as @himdel pointed out)

[himdel]

Thanks, tested, this fixes the ui-classic Travis failures 👍 :)

[moolitayer]

@Ladas @cben @durandom can one of you also review this PR?

[moolitayer]

tree_node_spec.rb (and other tests calling descendants) effectively makes manageiq-ui-classic dependent on all the other repositories. Maybe we should make a test dependency ? (simplest/stupidest solution would be to copy the test?)

[miq-bot]

Checked commit moolitayer@6c91e60 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

[himdel]

@moolitayer I was wondering if we should fix the spec to not assume as much.

It now effectively calls itself for all ExtManagementSystem.descendants, so maybe the assumption that we can .new all of them is wrong..

(There's even a # FIXME: rewrite this to FactoryGirl before that klass.new.)

[moolitayer]

@moolitayer I was wondering if we should fix the spec to not assume as much.

That sounds so much better. I'm not sure the factory girl alone would save us. Maybe starting with a whitelist of objects for the test?

[himdel]

Maybe.. but the thing is, we do need to support making a tree node for almost all the entities in the system, because if it's visible somewhere in the ui, it's most likely in one tree or other too.

I'd rather have a foolproof way of creating a realistic testing object for each of those..

We could do a blacklist, but.. I guess there's probably a plan to expose MonitoringManager in the UI as well, right? At which point we would remove it from the blacklist and be back where we are now..

[moolitayer]

Well it is not meant to be shown in the UI (which is why I don't feel at ease with this PR). But if all the objects in the system can be created with new it makes some sense that monitoring managers would support that as well.

[moolitayer]

@himdel approve of this change?

[chessbyte]

@moolitayer I thought @himdel approved here

[chessbyte]

@moolitayer please label the PR as best you can

[moolitayer]

@moolitayer I thought @himdel approved here

Might look like the approval is only on fixing travis and not the PR.
Merging since @himdel approved in gitter and Travis is failing

[cben]

Retroactive 👍, fixing cross-repo Travis is crucial.

... creation with out a parent obeject (which should never happen in real life)

... the thing is, we do need to support making a tree node for almost all the entities in the system
... I'd rather have a foolproof way of creating a realistic testing object for each of those..

IIUC we're discussing 3 options:

1. UI tests will exclude managers that need a parent.
2. Real life requires a parent, tests don't supply parent => code "supports" nil (not really, just enough for test).
3. Real lift & test supply parent.

IMHO (3) is best. That's generally what we write factories for, except how can test know correct factory for each class?

• What if Manager had a method new_for_test that defaults to new but classes like this override it (one way to implement is actually call a factory)? There might be more idiomatic factory_girl solution...

[bdunne]

@moolitayer Any reason there's no milestone here?

[moolitayer]

@bdunne see #72 (comment)