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

Add some more goldenfile tests #614

Merged
merged 9 commits into from
Oct 31, 2023
Merged

Add some more goldenfile tests #614

merged 9 commits into from
Oct 31, 2023

Conversation

MustafaSaber
Copy link
Member

Follow up on #587 for better coverage.

worker_test.go Outdated Show resolved Hide resolved
worker_test.go Outdated Show resolved Hide resolved
worker_test.go Outdated Show resolved Hide resolved
worker_test.go Outdated Show resolved Hide resolved
@lucastt
Copy link
Contributor

lucastt commented Jun 20, 2023

So, when reading the expected CF templates I noticed something weird, they seem to reference values that would be received as parameters, but there is no place showing the values of these parameters. To confirm my theory I executed a vimdiff between both expected files: testdata/ingress_rg_shared_alb/expected.cf and testdata/ingress_rg_notshared_alb/expected.cf and they yield no diff, meaning they are exactly the same.

When I created the structure of these tests I didn't notice it because I just made two simple tests, on creating ALB and the other creating NLB, which yield different templates. But now I see there may be a problem in the way I built these golden file tests. 🤔

Did I get something wrong?

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please cleanup these changes (do not commit these)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the newline better for git diff?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@szuecs let's discuss this to see how to move forward.

@lucastt
Copy link
Contributor

lucastt commented Jul 13, 2023

So, when reading the expected CF templates I noticed something weird, they seem to reference values that would be received as parameters, but there is no place showing the values of these parameters. To confirm my theory I executed a vimdiff between both expected files: testdata/ingress_rg_shared_alb/expected.cf and testdata/ingress_rg_notshared_alb/expected.cf and they yield no diff, meaning they are exactly the same.

When I created the structure of these tests I didn't notice it because I just made two simple tests, on creating ALB and the other creating NLB, which yield different templates. But now I see there may be a problem in the way I built these golden file tests. 🤔

Did I get something wrong?

Created a new issue for this. #626

@MustafaSaber MustafaSaber self-assigned this Aug 10, 2023
@lucastt
Copy link
Contributor

lucastt commented Oct 26, 2023

👍

@szuecs
Copy link
Member

szuecs commented Oct 27, 2023

@MustafaSaber the changes are great progress for tests!
If you do nonshared / share, please keep routegroup in ingress with annotations in-sync.
You can change the .kube file (at least in skipper tests) to configure the defaults of the controller and then test defaults, too. From my pov, to merge the only thing to make sure is that the tests are consistent in their annotation use.

@MustafaSaber
Copy link
Member Author

Thanks @lucastt for taking over 🙏!

aws/fake/cf.go Outdated Show resolved Hide resolved
@lucastt
Copy link
Contributor

lucastt commented Oct 31, 2023

👍

aws/fake/cf.go Outdated Show resolved Hide resolved
Signed-off-by: Lucas Thiesen <[email protected]>
@lucastt
Copy link
Contributor

lucastt commented Oct 31, 2023

👍

1 similar comment
@szuecs
Copy link
Member

szuecs commented Oct 31, 2023

👍

@szuecs szuecs merged commit dd95fc2 into master Oct 31, 2023
9 checks passed
@szuecs szuecs deleted the more-tests branch October 31, 2023 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants