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

Clean up sink_test.go #394

Closed
ncskier opened this issue Jan 29, 2020 · 5 comments
Closed

Clean up sink_test.go #394

ncskier opened this issue Jan 29, 2020 · 5 comments
Assignees
Labels
area/testing Issues or PRs related to testing kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@ncskier
Copy link
Member

ncskier commented Jan 29, 2020

The sink_test.go file has a lot of messy tests with repeated code (especially for the HandleEvent() function). It's difficult to write clean tests for this file, because it's pretty high-level. However, I think that we can clean up some of the code, and hopefully use some table driven tests to abstract away some of the testing logic.

@dibyom
Copy link
Member

dibyom commented Jan 30, 2020

/kind cleanup

@tekton-robot tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jan 30, 2020
@dibyom
Copy link
Member

dibyom commented Jan 30, 2020

/area testing

@tekton-robot tekton-robot added the area/testing Issues or PRs related to testing label Jan 30, 2020
@dibyom
Copy link
Member

dibyom commented Feb 4, 2020

/assign

@dibyom dibyom mentioned this issue Feb 4, 2020
3 tasks
dibyom added a commit to dibyom/triggers that referenced this issue Feb 18, 2020
This commit refactors sink_test:
1. Common setup/comparison extracted to helper methods
2. Removes some redundant params from the test resources
3. Remove the test that checks that we respond in 1s or less.
Since we are using fake clients in these examples, this test is
not as useful as it being in an e2e test.

Part of tektoncd#394
dibyom added a commit to dibyom/triggers that referenced this issue Feb 18, 2020
This commit refactors sink_test:
1. Common setup/comparison extracted to helper methods
2. Removes some redundant params from the test resources
3. Remove the test that checks that we respond in 1s or less.
Since we are using fake clients in these examples, this test is
not as useful as it being in an e2e test.

Part of tektoncd#394

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Feb 18, 2020
This commit refactors sink_test:
1. Common setup/comparison extracted to helper methods
2. Removes some redundant params from the test resources
3. Remove the test that checks that we respond in 1s or less.
Since we are using fake clients in these examples, this test is
not as useful as it being in an e2e test.

Part of tektoncd#394

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Feb 28, 2020
This commit refactors sink_test:
1. Common setup/comparison extracted to helper methods
2. Removes some redundant params from the test resources
3. Remove the test that checks that we respond in 1s or less.
Since we are using fake clients in these examples, this test is
not as useful as it being in an e2e test.

Part of tektoncd#394

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
dibyom added a commit to dibyom/triggers that referenced this issue Feb 28, 2020
This commit refactors sink_test:
1. Common setup/comparison extracted to helper methods
2. Removes some redundant params from the test resources
3. Remove the test that checks that we respond in 1s or less.
Since we are using fake clients in these examples, this test is
not as useful as it being in an e2e test.

Part of tektoncd#394

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
tekton-robot pushed a commit that referenced this issue Mar 2, 2020
This commit refactors sink_test:
1. Common setup/comparison extracted to helper methods
2. Removes some redundant params from the test resources
3. Remove the test that checks that we respond in 1s or less.
Since we are using fake clients in these examples, this test is
not as useful as it being in an e2e test.

Part of #394

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
@dibyom
Copy link
Member

dibyom commented Apr 27, 2020

#413 fixed most of this -- most setup is now abstracted out into helper functions. Table driven tests will be a bit harder here given the variety of things were are asserting.
Closing for now
/close

@tekton-robot
Copy link

@dibyom: Closing this issue.

In response to this:

#413 fixed most of this -- most setup is now abstracted out into helper functions. Table driven tests will be a bit harder here given the variety of things were are asserting.
Closing for now
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues or PRs related to testing kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

No branches or pull requests

3 participants