Passing capacity hash to values broke PV screen

[nimrodshn]

Passing capacity hash to values broke the PV summary screen - table rows are expected to be an array.

cc: @cben @himdel @martinpovolny @oourfali
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1594567

Before:

After:

[cben]

Ideally this would show "10G".
don't remember the helper for that...

[martinpovolny]

@nimrodshn : Thank you for the fix.

This is obviously a regression that I caused while converting all the detail screens to use React components for display.

I wonder what is the best way to test this. I mean we have so many screens like this one and very little test coverage. It's impossible to test all of them when something changes and also it's easy to break if there's no test.

Now the javascript components have a interface with some type descriptions (using PropTypes) and there are classes on the Ruby side. It should be no big deal to have a simple test for this screen.

[nimrodshn]

@martinpovolny Do you mind pointing out to some relevant tests? I haven't touched on miq code in a while..

[martinpovolny]

@martinpovolny Do you mind pointing out to some relevant tests? I haven't touched on miq code in a while..

That's what I am wondering about. What it the best way. Now the components have their own tests -- https://github.com/ManageIQ/react-ui-components/tree/master/src/textual_summary/tests -- that is for the rendering part.

We can test that for a given @record the proper JSON is generated. That is called from app/views/layouts/_textual_groups_generic.html.haml:

ManageIQ.component.componentFactory('TextualSummaryWrapper', '#textual_summary', {summary: #{process_textual_info(textual_group_list, @record).to_json}});

In that place and to stay in the Ruby world what is important is that the result of

process_textual_info(textual_group_list, @record)

is what is expected. From there the data is passed to the React component and the rendering of that data should be covered by react-ui-components/tree/master/src/textual_summary/tests.

So I think that the best would be to have an example record -- a PersistentVolume created by FactoryBot, run it through the process_textual_info in the right context and make assertions about the resulting (Ruby) hash.

Previously we had a few (and ugly) tests that matched text in the page. Nothing really systematic I am afraid.

p.s. @nimrodshn : I don't want to force you to solve this in this PR and block you from other work, but I'd like to have a clear path forward and a way to limit regressions in the future.

@skateman : What do you think? Can you help with this?

[martinpovolny]

@nimrodshn : here's a deal.

1. I merge this w/o a test.
2. You write down for me what factory with what arguments should be used to populate the @record and what are the important attributes that I should test.
3. I write some test for this screen (and some more screens) and have it discussed with @skateman and other people to establish a pattern

Does it work for you?

[martinpovolny]

Restarting travis.

[martinpovolny]

Please, fix the style issues, then I can merge this.

[nimrodshn]

@martinpovolny you have yourself a deal my friend 😸 👍 (Lets talk privately on gitter)

[miq-bot]

Checked commit nimrodshn@52d86fa with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

[nimrodshn]

@martinpovolny
The factory should look like:

persistent_volume = FactoryGirl.build(
      :persistent_volume,
      :name                    => "persistent_volume",
      :capacity => {:storage => 123_456_789, :foo => "something"}
    )

In current version of kubernetes there is only one key in :capacity hash (namely storage) although the UI shouldn't assume this.

So calling process_textual_info on the above pv should result with an object with attribute: rows => [["storage", "123456789"], ["foo", "something"]]

[martinpovolny]

Thx!