Restore all previous TreeNodeBuilder#build functionality and add specs #12060

NickLaMuro · 2016-10-19T18:33:35Z

Purpose

Two fold:

Adds tests based off the previous implementation so we are confident with any future refactorings that the functionality is retained.
Simplifies the TreeNodeBuilder#build method to resolve purely by .base_class method of the object's class in question.

Further information regarding the process for building the specs and rational can be found in the commit messages and in the links below

Links

NickLaMuro · 2016-10-19T18:34:32Z

@miq-bot add_label bug, ui, refactoring
@miq-bot assign @himdel

himdel · 2016-10-20T16:22:14Z

Wow, really nice :) I'll do a thorough review tomorrow but I love those tests! <3 :)

Btw.. looks like you found a bug, seems like a lot of those entities don't have a defined prefix (note to self - fix that :D) (all those :key => "-#{vm.name}", where the string starts with just the dash).

NickLaMuro · 2016-10-20T16:44:16Z

@himdel No, not a bug, it is actually how the .build_object_id is implemented. I, and the previous test authors, are mainly using .build for a factory instead of .create (for speed reasons so we aren't hitting the DB... arguably we maybe shouldn't).

.build_object_id, when the .id is nil, will just use the .name of the object and return it. Unfortunately, not all the objects that go through this have a .name attribute, so some we are forced to .create because of that.

himdel · 2016-10-20T16:47:38Z

@NickLaMuro not what I meant..

Compare..

expect(node).to eq(
  :key    => "cd-#{compliance.id}",
)

with

expect(node).to eq(
  :key    => "-#{condition.name}",
)

All those starting with a dash should have an entry in X_TREE_NODE_PREFIXES. but don't right now.. But it's a separate issue, it would still mostly work, but have obscure bugs in lazy trees and whenever you have to process the id in any way.

NickLaMuro · 2016-10-20T16:54:03Z

I think we are still talking about the same thing... check the FactoryGirl.build v.s. FactoryGirl.create in each of those tests you just referenced (ids aren't created when you do a .build, it is just an class instantiation, or .new).

Regardless, I have to fix these tests that are borked, and I want you to have your evening back. See you tomorrow! 😄

himdel · 2016-10-20T16:55:27Z

Oh! You're right, it could be that :)

But agreed, tomorrow is a day too :)

Adds tests for (almost) every possible class the could be run through the `.build` method from our application, and confirms the result object from it (instead of just confirming it isn't `nil`). This will allow for confidence in refactoring in the future to move away from the current `hash`/`case` setup and confirm functionality. These specs can be moved elsewhere if such a refactoring were to take place, but for now, this allows us to confirm existing functionality with the current behavior. The models added here were gathered through the following method: 1. First, the following commit was checked out to revert the changes to TreeNodeBuilder prior to converting the large `case` in the `.build` to a hash: $ git checkout ee78d9c -- app/presenters/tree_node_builder.rb 2. Then each of the keys in the `case` was run through the following code in a `rails console` to determine which models needed to be tested against: irb(main):001:0> checker_proc = Proc.new {|klass| klass.descendants.map(&:base_class).map(&:to_s).uniq } => #<Proc:0x007f8febe34360@(irb):1> irb(main):002:0> descendants_proc = Proc.new {|klass| klass.descendants.map(&:to_s) } => #<Proc:0x007f8feac69748@(irb):2> irb(main):003:0> check = Proc.new {|klass| puts descendants_proc.call(klass).inspect; puts checker_proc.call(klass).inspect; nil } => #<Proc:0x007f8feaf4b0d8@(irb):3> irb(main):004:0> check.call(AvailabilityZone) ["ManageIQ::Providers::Azure::CloudManager::AvailabilityZone", "ManageIQ::Providers::Google::CloudManager::AvailabilityZone", "ManageIQ::Providers::Openstack::CloudManager::AvailabilityZone", "ManageIQ::Providers::Vmware::CloudManager::AvailabilityZone", "ManageIQ::Providers::Amazon::CloudManager::AvailabilityZone", "ManageIQ::Providers::Openstack::CloudManager::AvailabilityZoneNull"] ["AvailabilityZone"] => nil irb(main):004:0> AvailabilityZone.base_class This allowed it to be determined the class that needed to be included in the specs to confirm previous behavior prior to the commit. It was then used to validate that the new changes worked in the same fashion. Also of note: The "VmOrTemplate node" context in the specs was referring to the `MiqTemplate` key from the original case statement (changed in 54e254e for a bug fix), but was changed to reflect the current implementation (will be updated in the future as well, since the `Vm` key is also a subclass of `VmOrTemplate`).

`Vm` is just a subclass of `VmOrTemplate` as well, so this makes the lookup simpler by just needing the `.base_class` of `object`, and reduces the code in the class. Also restructures the tests to match the new key structure. A few things of note: * VmServer and VmXen are now added as part of the specs for the Vm classes in the "VmOrTemplate node" context. This is because they didn't work prior because they didn't have a lowest level module of `Vm`, and they inherited from VmOrTemplate, which up until this commit, they produced the incorrect node hash for Vm classes. * The vm_node method was re-worked, and removed the logic for using a separate picture if the "Vm" was a template. As the previous sentence probably shows, for the previous usage of this method, everything passed into it would never be a template so this code was never executed to use the "template" images. * The `VmOrTemplate` proc (again, originally the key was `MiqTemplate` prior to the 54e254e changes) originally was using nearly similar code as `Vm` key, but didn't include a tooltip. This is why the slight change to the conditional exists in `vm_node`. * TreeNodeBuilderDatacenter#vm_node was also updated to reflect the new changes that were made in the base class, but retained the difference in tool tip for that subclass (every node includes " (Click to view)" in that class).

These are the 5 classes that inherit from `OrchestrationTemplate`: OrchestrationTemplateCfn OrchestrationTemplateHot OrchestrationTemplateAzure OrchestrationTemplateVnfd ManageIQ::Providers::Vmware::CloudManager::OrchestrationTemplate Yeah, I don't like that last one either... Since they all are mostly the same, and can use `.base_class` to resolve the `OrchestrationTemplate` key, these have been combined into one key and a helper method has been created for generating the node.

Switching VmdbTableEvm to it's `base_class`, `VmdbTable`, allows for a simpler proc resolution, and is the last key that needs to be found using `.base_class`.

This was a hack to handle keys in the `TreeNodeBuilder::BUILD_NODE_HASH` that weren't base classes. These have now all been converted, so this works without that hack, and only the `Hash` key (which doesn't respond to `.base_class`) should respond to it this way.

NickLaMuro · 2016-10-20T21:01:27Z

Alright, turns out I was wrong, and you were right!

Two of the failing tests on CI caught them, and they were for DialogTab and DialogField. Not sure how important they are in this dictionary though.

Makes me wonder if we should be doing a .create on all of these though...

miq-bot · 2016-10-20T21:02:27Z

Checked commits NickLaMuro/manageiq@a7f69bf~...6da49fa with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
11 files checked, 0 offenses detected
Everything looks good. 🏆

himdel · 2016-10-21T16:15:24Z

Dialog* shouldn't need to be in the dictionary, since I think we only work with them in the dialog editor form, which is a bit special, but then again, safer to have all of them there, just to be sure.. But it can be a separate issue, Cc @ZitaNemeckova so as not to forget during that tree's conversion.. ;)

As for create vs build.. ¯_(ツ)_/¯ maybe? Up to you.. :)

himdel · 2016-10-21T16:51:09Z

Tested in the UI, looked in the code, can't find anything wrong 👍 maybe someone else will :)

himdel · 2016-10-21T16:52:28Z

Euwe backporting order:
#11604
#11890
#12060 (this one)

himdel · 2016-10-21T17:03:12Z

@ZitaNemeckova if you could go through the trees as well to see if you notice anything weird... Thanks!

Restore all previous TreeNodeBuilder#build functionality and add specs (cherry picked from commit 7f18af0)

chessbyte · 2016-10-21T22:01:07Z

Euwe Backport details:

$ git log -1
commit caace3faa2631d7efc8a0a8b75cba912a0f8a576
Author: Martin Hradil <himdel@seznam.cz>
Date:   Fri Oct 21 16:50:14 2016 +0000

    Merge pull request #12060 from NickLaMuro/proper_tree_node_builder_keys

    Restore all previous TreeNodeBuilder#build functionality and add specs
    (cherry picked from commit 7f18af009698082dcc9232329ecef85493b83b1c)

miq-bot added bug refactoring ui labels Oct 19, 2016

miq-bot assigned himdel Oct 19, 2016

NickLaMuro force-pushed the proper_tree_node_builder_keys branch 3 times, most recently from 55d1d8c to 6c70f6d Compare October 20, 2016 15:41

NickLaMuro force-pushed the proper_tree_node_builder_keys branch from 6c70f6d to a6592ce Compare October 20, 2016 17:14

NickLaMuro added 5 commits October 20, 2016 15:59

Make VmdbTableEvm a base class key

a61475c

Switching VmdbTableEvm to it's `base_class`, `VmdbTable`, allows for a simpler proc resolution, and is the last key that needs to be found using `.base_class`.

NickLaMuro force-pushed the proper_tree_node_builder_keys branch from a6592ce to 6da49fa Compare October 20, 2016 20:59

himdel mentioned this pull request Oct 21, 2016

Use VmOrTemplate instead of MiqTemplate for lookup #11890

Merged

himdel merged commit 7f18af0 into ManageIQ:master Oct 21, 2016

himdel added the euwe/yes label Oct 21, 2016

himdel modified the milestones: Roadmap, Sprint 48 Ending Oct 24, 2016 Oct 21, 2016

himdel mentioned this pull request Oct 21, 2016

Move most of TreeNodeBuilder#build to hash lookup #11604

Merged

chessbyte pushed a commit that referenced this pull request Oct 21, 2016

Merge pull request #12060 from NickLaMuro/proper_tree_node_builder_keys

caace3f

Restore all previous TreeNodeBuilder#build functionality and add specs (cherry picked from commit 7f18af0)

chessbyte added euwe/backported and removed euwe/yes labels Oct 21, 2016

This was referenced Oct 22, 2016

[EUWE] Remove physical infra reference to make tests pass #12119

Closed

[EUWE] Remove reference to :ems_physical_infra #12120

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore all previous TreeNodeBuilder#build functionality and add specs #12060

Restore all previous TreeNodeBuilder#build functionality and add specs #12060

NickLaMuro commented Oct 19, 2016 •

edited

NickLaMuro commented Oct 19, 2016

himdel commented Oct 20, 2016

NickLaMuro commented Oct 20, 2016

himdel commented Oct 20, 2016 •

edited

NickLaMuro commented Oct 20, 2016

himdel commented Oct 20, 2016

NickLaMuro commented Oct 20, 2016

miq-bot commented Oct 20, 2016

himdel commented Oct 21, 2016

himdel commented Oct 21, 2016

himdel commented Oct 21, 2016

himdel commented Oct 21, 2016

chessbyte commented Oct 21, 2016

Restore all previous TreeNodeBuilder#build functionality and add specs #12060

Restore all previous TreeNodeBuilder#build functionality and add specs #12060

Conversation

NickLaMuro commented Oct 19, 2016 • edited

Purpose

Links

NickLaMuro commented Oct 19, 2016

himdel commented Oct 20, 2016

NickLaMuro commented Oct 20, 2016

himdel commented Oct 20, 2016 • edited

NickLaMuro commented Oct 20, 2016

himdel commented Oct 20, 2016

NickLaMuro commented Oct 20, 2016

miq-bot commented Oct 20, 2016

himdel commented Oct 21, 2016

himdel commented Oct 21, 2016

himdel commented Oct 21, 2016

himdel commented Oct 21, 2016

chessbyte commented Oct 21, 2016

NickLaMuro commented Oct 19, 2016 •

edited

himdel commented Oct 20, 2016 •

edited