Tests for children of ListMessage express incorrect behaviours

[blgm]

Thanks for submitting an issue to cloud_controller_ng. We are always trying to improve! To help us, please fill out the following template.

Issue

There is an inconsistency in the understanding of how a child of ListMessage should behave if it receives a non-array value for a field of type array. There are a large number of tests (about 100) that talk about behaviours that are incorrect. These behaviours occur in unit tests, but cannot occur in reality.

Context

To take one example, the TaskListMessage spec suggests that validation should fail if names is not an array: https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/spec/unit/messages/tasks_list_message_spec.rb#L166-L170

But the implementation suggests that if names is not an array, it should be converted to an array: https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/messages/tasks_list_message.rb#L32-L34 and: https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/messages/list_message.rb#L75-L81

So it looks as if the test (above) should fail because names will automatically be converted to an array without validation failure. But the test passes because in the unit test params is a plain hash where the key is a symbol (:names), while the implementation tries to look up the (string) key "names", and therefore does not do the conversion.

In reality, params will be a HashWithIndifferentAccess, so the behaviour will be to convert names (or :names) because this data type does not distinguish between strings and symbols. We can see the HashWithIndifferentAccess being created here: https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/controllers/v3/application_controller.rb#L63-L65

The test expresses a behaviour that is simply incorrect, and makes it harder to understand what is going on.

We have tried changing the implementation to always use symbolised keys, and this causes over 100 unit test failure. We think most of those unit tests are simply invalid.

Steps to Reproduce

Change this: https://github.com/cloudfoundry/cloud_controller_ng/blob/72d262f52187a2c3427fbc6b4958b783f8140acf/app/messages/list_message.rb#L75-L82

to this:

def self.from_params(params, to_array_keys) 
   opts = params.dup.symbolize_keys
   to_array_keys.each do |attribute| 
     to_array! opts, attribute.to_sym
   end 

   message = new(opts)
   message.requirements = parse_label_selector(opts[:label_selector]) if message.requested?(:label_selector)

Expected result

We would expect the above change to leave behaviour unaffected.

Current result

The above change causes 190 tests to fail.

Possible Fix

We think that there are a number of unit tests that should be deleted or updated.

We also think we should clarify whether the Rails query_parameters hash values can ever contain array types. Many unit tests assume this is the case.

We also think that given query_params always returns a HashWithIndifferentAccess, unit tests that fake this should use the same type, and the implementation of from_params() should check that it receives this type.

[cf-gitbot]

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/170360734

The labels on this github issue will be updated when the story is started.

[cwlbraa]

Ick... this is some ugly test tech debt. We might want to come up with a plan to pay it down?