fixes bug where downloading a cookbook version would break if the coo… #1452
Conversation
…kbook version had one metric but not the other associated with it Signed-off-by: Nell Shamrell <[email protected]>
Signed-off-by: Nell Shamrell <[email protected]>
| json.failed collaborator_num_metric_result(cookbook_version).failure | ||
| json.feedback collaborator_num_metric_result(cookbook_version).feedback | ||
| end | ||
| json.quality_metrics @cookbook_version_metrics do |metric| |
There was a problem hiding this comment.
I think the iteration done by the builder thingie used here alone will take care of the !nil? and any? case. If @cookbook_version_metrics is nil or [], the iteration will be a no-op.
Whether that is correct or not, I suggest the if above also include (or only be) a feature flag check for fieri being enabled. (Under the philosophy that if fieri is disabled, like in the human UI, don't show metrics regardless of their presence.)
There was a problem hiding this comment.
Agreed, will add that and test.
Signed-off-by: Nell Shamrell <[email protected]>
| context 'when the cookbook has only the foodcritic metric' do | ||
| before do | ||
| collab_result.destroy | ||
| expect(cookbook_version.metric_results.count).to eq(1) |
There was a problem hiding this comment.
A couple of thoughts here:
- An expectation in a
beforesetup block is a little ..er.. unexpected. I can understand putting this in during development as a confirmation that the setup has achieved the desired test start state, but I recommend we not keep this line. - If we do keep this line, we could protect all tests in this context from breaking when we have 3 or more metrics in the system if we expect a change in the count instead of a hard-coded count.
expect {
collab_result.destroy
}.to change {
cookbook_version.metric_results.count
}.by(-1)
... but then that ends up looking even more like something that is odd to put in a before setup block.
There was a problem hiding this comment.
That was meant for development only, removing.
| expect(signature(json_body)).to include(quality_metrics) | ||
| it 'does not return quality metrics for the cookbook version' do | ||
| get "/api/v1/cookbooks/#{cookbook.name}/versions/#{cookbook_version.version}" | ||
| expect(JSON.parse(response.body)).to_not include(quality_metrics) |
There was a problem hiding this comment.
What do you think about making this only check for a key named "quality_metrics" in the returned JSON so that the test does not depend on the whole structure of the results?
There was a problem hiding this comment.
We could do that...but I think that this is a little clearer and I don't think it costs us much in terms of time and memory.
Signed-off-by: Nell Shamrell <[email protected]>
|
👍 Pending test success. |
…kbook version had one metric but not the other associated with it
Signed-off-by: Nell Shamrell [email protected]