-
Notifications
You must be signed in to change notification settings - Fork 417
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
Cleanup Reconciler Tests in order to pick up Genreconciler updates #706
Conversation
The following is the coverage report on the affected files.
|
The following is the coverage report on the affected files.
|
We had separate tests for reconcileService and reconcileDeployment. These tests were hard to port with the genreconciler update(tektoncd#635) which changes the Reconciler interface. Instead of testing these unexported functions, I merged the test cases for these into the existing TestReconcile function Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
The following is the coverage report on the affected files.
|
The reconciler tests used variables of the form service1, deployment2 etc. making it hard to figure out what those objects actually did. This change does the following: 1. Adds more descriptive names e.g. `eventListenerWithNodeSelector` instead of `eventlistener6` 2. Adds helper functions `makeEL`, `makeDeployment`, `makeService` to create the above objects instead of deep-copying and changing values 3. Fixes a bug I found in the process: The tests were setting the generatedResourceName in the status of the pre-reconciled resource. Instead it should be set by the Reconciler. Without this, we weren't actually testing if the status contained the right `generatedResourceName`. Signed-off-by: Dibyo Mukherjee <dibyo@google.com>
The following is the coverage report on the affected files.
|
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bleh - This is still a bit hard to follow, since the transformation from start to end state are still somewhat obscured since they are not localized with the tests themselves.
That said, this is definitely an improvement from the previous state, so lgtm!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wlynch The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Yeap, next PR is to fix that 😄 |
/test tekton-triggers-unit-tests |
Changes
This PR has two commits and is needed for #635 :
Merge service and deployment tests into TestReconcile
We had separate tests for reconcileService and reconcileDeployment. These tests
were hard to port with the genreconciler update(#635) which changes the Reconciler
interface. Instead of testing these unexported functions, I merged the test
cases for these into the existing TestReconcile function
Make reconciler tests more readable …
The reconciler tests used variables of the form service1, deployment2 etc.
making it hard to figure out what those objects actually did. This change does
the following:
Adds more descriptive names e.g.
eventListenerWithNodeSelector
instead ofeventlistener6
Adds helper functions
makeEL
,makeDeployment
,makeService
to create the above objects instead of deep-copying and changingvalues
Fixes a bug I found in the process: The tests were setting the
generatedResourceName in the status of the pre-reconciled resource. Instead it
should be set by the Reconciler. Without this, we weren't actually testing if
the status contained the right
generatedResourceName
/kind cleanup
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes