Fix overfitting label tests #2072
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. Maintainer note: consult the runbook for dealing with any breaking changes. |
| @@ -370,6 +393,35 @@ func (st labelsState) validateTransitionTo(t *testing.T, st2 labelsState) { | |||
| integration.ProgramTest(t, &opts) | |||
| } | |||
|
|
|||
| func (st labelsState) expectedLabelsPRC(prev labelsState) map[string]string { | |||
| // Note that the upstream provider actually takes a "" value for a label to mean "keep the previous value". | |||
There was a problem hiding this comment.
This is highly suspect, do you have a code reference? This reproduces in TF? I can think of strong customer feedback in AWS that empty tag values should not be specially treated, suspect the same may be the case here.
There was a problem hiding this comment.
I see this is present in TF. Still in AWS we were asked to fix it even though TF had this problem. The PR looks good to me but I'd request tracking this problem in the issue tracker so that we can at least call this out to users appropriately when PRC is rolling out.
There was a problem hiding this comment.
I'll open an issue and link it in the rollout issue, thanks
During PlanResourceChange these tests was discovered to fail. Looks like this is actually an upstream bug - the provider takes empty string for a label to mean "keep previous value". This was reproed in TF.
I've adjusted the tests to take this into account - they should now work for both PRC and non-PRC. We can remove the non-PRC bit after enabling PRC by default.
fixes #2078