Fix test for MW datasource - missing parent

[tumido]

Fix broken UI classic Travis where test for MW Datasource controller was failing due to missing parent MW server.

Found thanks to: #16611
Related: ManageIQ/manageiq-ui-classic#2972, ManageIQ/manageiq-ui-classic#3040, ManageIQ/manageiq-ui-classic#3044

@himdel, please review

[tumido]

@miq-bot add_label test, bug, gaprindashvili/yes

[miq-bot]

Checked commit tumido@4a75d1f with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

[tumido]

@chrisarcand, please review. This is fixing broken Travis for manageiq-ui-classic on master, which became broken after my #16611. (It unintentionally revealed broken factory used by UI and I didn't caught it.)

[abonas]

@miq-bot add_label providers/middleware

[chrisarcand]

Why specifically is it in this factory and not the parent if the general model specifies a delegation that potentially breaks things?

[tumido]

I did it this way, because the factory is called hawkular_middleware_datasource_initialized. That means, you want it to be ready to use. However, the parent one would assume you want a custom datasource, which you define on your own. And since for example all operations done on middleware datasource are done via mw server, you should have it already defined in the test context as well.

[chrisarcand]

I did it this way, because the factory is called hawkular_middleware_datasource_initialized. That means, you want it to be ready to use. However, the parent one would assume you want a custom datasource, which you define on your own.

This misses the point of inheritance and overriding in factories; FactoryGirl is specifically intended to allow you to have a fully working object (it has everything it needs to function) at the top level and customize as needed without redefining an entirely new factory. Don't bother changing it at this point, but for your own information...

:hawkular_middleware_datasource_initialized shouldn't exist, and the :hawkular_middleware_datasource should just have middleware_server, etc. If you want a custom datasource, you do define it on your own, and don't need a separate factory for it:

# Give me a hawkular datasource that works, complete with the server:
FactoryGirl.create(:hawkular_middleware_datasource)

# Give me a hawkular datasource but I want to specify a custom server:
FactoryGirl.create(:hawkular_middleware_datasource, :middleware_server => my_custom_middleware_server, :nativeid => "blah")

[jrafanie]

Sorry, I'm not understanding. Are we saying that hawkular_middleware_datasource is still broken in some situations because we're not specifying a required middleware_server?

[himdel]

I'd even say maybe :middleware_datasource is the broken one, since even MiddlewareDatasource belongs_to :middleware_server.

That said, all other uses of hawkular_middleware_datasource do provide either middleware_server or server_id, except for:

manageiq-ui-classic/spec/helpers/application_helper_spec.rb
1301:        FactoryGirl.create(:hawkular_middleware_datasource, :ext_management_system => ems, :name => "Test Middleware")

[himdel]

(as for :middleware_datasource, there's one use which provides middleware_server, and 3 which don't:

manageiq-api/spec/requests/middleware_datasources_spec.rb
2:  let(:datasource) { FactoryGirl.create(:middleware_datasource) }

manageiq-api/spec/requests/collections_spec.rb
152:      FactoryGirl.create(:middleware_datasource)
480:      FactoryGirl.create(:middleware_datasource)

)

[tumido]

@jrafanie, no, we have currently no other testcase which would be broken by this. But generally speaking yes, it can be used in "broken" way. But we have many factories which allows that unfortunately (the closest one is :middleware_deployment for example).

@chrisarcand, I was also surprised by existence of the :hawkular_middleware_datasource_initialized however in order to fix the Travis I did the minimally invasive thing around. Sure I'm aware you should define own values, and honestly it was my first attempt to fix this in ManageIQ/manageiq-ui-classic#3044. And by request it was changed to the factory change...

I assume the best approach would be to change the top level :middleware_datasource to include an association to :middleware_server, is that correct?

[chrisarcand]

If we want to talk very objectively about how it should be, the top middleware datasource should provide everything needed for a middleware datasource to function, yes; and :hawkular should be a FactoryGirl/Bot trait. That last part I hadn't mentioned because we regrettably do this 'redefining a new factory instead of using traits' all over the codebase :)

however in order to fix the Travis I did the minimally invasive thing around.

Yup, I respect that you're not the one who introduced this _initialized thing and we're venturing out of scope 😃 I would have asked you to change things around had I the opportunity to review this (Good morning from the American Central timezone, btw), but as I said, don't bother now that it's already merged and fixes your specific issue.

[tumido]

@chrisarcand, Honestly, I do appreciate your review! And of course it would be nice to have it done properly. I'm gonna take a look into it and update at least the middleware factories to truly provide what they should. 😉

[jrafanie]

@tumido 👍 Yes, doing a tactical fix in order to get travis green is most important especially if the "right" fix requires some discussion. You did the right thing. A followup PR to fix the parent factory is fine now that travis can has been fixed.

[martinpovolny]

I would have asked you to change things around had I the opportunity to review this (Good morning from the American Central timezone, btw), but as I said, don't bother now that it's already merged and fixes your specific issue.

Right, I merged this w/o waiting for you. Please, accept my apology, @chrisarcand.

The priority for me was to get master green so that we can carry on with day to day work. And the simple fix did right that.

I believe a well discussed and designed solution to the problem can be implemented in a follow up patch w/o the feeling that we need to hurry because Travis is red on all PRs.

I had no intention to avoid or ignore your feedback ;-)

[chrisarcand]

It's fine, I know everything was red and terrible. ❤️ Factories are unfortunately so tightly coupled between the two repos that these sorts of quickfixes are common.

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 8c1a5a2c8a0ed6f74d50daabd0cb7be16cf7bc01
Author: Martin Povolny <mpovolny@redhat.com>
Date:   Wed Dec 13 14:07:36 2017 +0100

    Merge pull request #16653 from tumido/fix_travis_mw_datasource
    
    Fix test for MW datasource - missing parent
    (cherry picked from commit 0fa89e83f48923022ab9aae5fb9c9f53caff7396)