Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Find root cause for why reconciler tests were not failing #1245

Closed
dibyom opened this issue Oct 21, 2021 · 1 comment · Fixed by #1246
Closed

Find root cause for why reconciler tests were not failing #1245

dibyom opened this issue Oct 21, 2021 · 1 comment · Fixed by #1246
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@dibyom
Copy link
Member

dibyom commented Oct 21, 2021

Expected Behavior

Reconciler tests should fail when expected test output does not match actual output.

Actual Behavior

Our reconciler tests were passing when expected test output did not match actual output 😱 😱

While working on #1244 I noticed that our reconciler tests were ignoring metav1.ObjectMeta.Finalizers for test output comparison. I decided to remove that line since we are no longer using Finalizers since #1207

On removing that line though, I noticed a bunch of our tests started failing. Specifically the expected container args field in the deployment did not match the actual container args in the deployment. I verified that the actual container args were correct and that the expected values in the test were wrong. We did not update the tests when we moved the args field from -foo, value to the --foo=value pattern.

I fixed the tests but did not investigate the root cause.

We should find out why our unit tests were wrong i.e not comparing the values correctly.

@dibyom dibyom added the kind/bug Categorizes issue or PR as related to a bug. label Oct 21, 2021
@dibyom
Copy link
Member Author

dibyom commented Oct 21, 2021

@wlynch @lbernick and I were looking into this. We found that the main cause was the ignore function we were using.

We were using cmpopts.IgnoreTypes which "ignores all values assignable to certain types, which are specified by passing in a value of each type."

ObjectMeta.Finalizers has type []string which meant that the test was ignore all []string types in comparison (like the container args) field.

To fix, we should move to cmpopts.IgnoreFields which ignores a given field within a specific struct type.

@wlynch also found that we were using IgnoreTypes in a bunch of pipelines tests too for ignoring the LastTransitionTime field in status.Conditions. We should fix that too.

@dibyom dibyom self-assigned this Oct 21, 2021
dibyom added a commit to dibyom/triggers that referenced this issue Oct 21, 2021
IgnoreTypes will ignore all values of the given type i.e. for apis.Condition{}.LastTransitionTime.Inner.Time,
it will ignore all instances of time.Time. Using `cmpopts.IgnoreFields` will only ignore the specific field.
This was the root cause behind tektoncd#1245 since we were ignoring all []string values
while trying to ignore only objectMeta.Finalizer (which is of type []string)

Signed-off-by: Dibyo Mukherjee <[email protected]>

Fixes tektoncd#1245
dibyom added a commit to dibyom/triggers that referenced this issue Oct 21, 2021
IgnoreTypes will ignore all values of the given type i.e. for apis.Condition{}.LastTransitionTime.Inner.Time,
it will ignore all instances of time.Time. Using `cmpopts.IgnoreFields` will only ignore the specific field.
This was the root cause behind tektoncd#1245 since we were ignoring all []string values
while trying to ignore only objectMeta.Finalizer (which is of type []string)

Fixes tektoncd#1245

Signed-off-by: Dibyo Mukherjee <[email protected]>
tekton-robot pushed a commit that referenced this issue Oct 21, 2021
IgnoreTypes will ignore all values of the given type i.e. for apis.Condition{}.LastTransitionTime.Inner.Time,
it will ignore all instances of time.Time. Using `cmpopts.IgnoreFields` will only ignore the specific field.
This was the root cause behind #1245 since we were ignoring all []string values
while trying to ignore only objectMeta.Finalizer (which is of type []string)

Fixes #1245

Signed-off-by: Dibyo Mukherjee <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant