chore(tests): Add YAML-driven integration test framework for sources#6158
Conversation
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
2027d16 to
ec8f6da
Compare
Pull Request Test Coverage Report for Build 23136525461Details
💛 - Coveralls |
* master: fix(annotations): initialize annotation keys at declaration time (kubernetes-sigs#6159) chore(linter): unused params and functions linter (kubernetes-sigs#6142) docs(fqdn): use correct arguments order in FQDN Templating custom functions (kubernetes-sigs#6144)
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
7e64569 to
452a67b
Compare
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
a2b815f to
6e3b7ae
Compare
6e3b7ae to
bb4fdc9
Compare
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
/test pull-external-dns-unit-test |
| // Validate scenarios | ||
| for i, s := range scenarios.Scenarios { | ||
| if s.Description == "" { | ||
| return nil, fmt.Errorf("scenario %d (%q) is missing required field: description", i, s.Name) | ||
| } | ||
| } |
There was a problem hiding this comment.
Only description is required?
There was a problem hiding this comment.
added few more fields. I was thinking about adding github.com/go-playground/validator/v10. and
The struct tag approach would look like:
type Scenario struct {
Name string `json:"name" validate:"required"`
Description string `json:"description" validate:"required"`
Config ScenarioConfig `json:"config"`
...
}
And in LoadScenarios:
validate := validator.New()
for i, s := range scenarios.Scenarios {
if err := validate.Struct(s); err != nil {
return nil, fmt.Errorf("scenario %d (%q): %w", i, s.Name, err)
}
}
But for now probably overkill. Will see
| } | ||
| } | ||
|
|
||
| return &scenarios, nil | ||
| } |
There was a problem hiding this comment.
I would prefer to used file embedding, because editing the tests.yaml file does not invalidate the test cache, which then requires to run the test with go test -count=1.
Embedding it invalidates the cache when the file is modified.
Something like this:
package scenarios
import _ "embed"
//go:embed tests.yaml
var Tests []byte var testScenarios TestScenarios
if err := yaml.Unmarshal(scenarios.Tests, &testScenarios); err != nil {
return nil, err
}There was a problem hiding this comment.
Make sense. Implemented
| args := m.Called() | ||
| if args.Error(1) != nil { | ||
| return nil, args.Error(1) | ||
| } | ||
| return args.Get(0).(gateway.Interface), nil |
There was a problem hiding this comment.
Why not a not implemented error as in RESTConfig()? Or an explicit panic?
It should panic at m.Called() since there is no .On().Return for this method.
There was a problem hiding this comment.
At the moment is return nil, fmt.Errorf("RESTConfig: not implemented"). Basically, in real scenario is required, at the moment, we are using moscks directly. I simply have no use case at the moment.
Co-authored-by: vflaux <38909103+vflaux@users.noreply.github.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
Signed-off-by: ivan katliarchuk <ivan.katliarchuk@gmail.com>
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ivankatliarchuk The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What does it do ?
I'm going to add more tests in follow-up, in this PR focus is on adding the testing framework.
Main challanges this should help to solve
we have multiple layers like
wrappers are going to play key role in future, especially when we remove most of the providers and keep only webhook. Most of the logic is going to be happening in service postprocessors -> and this combination service + service + wrapper + wrapper needs a testing framework
The folder
tests/integration/toolkitrequire revisit, theres are some duplicates, in follow-up I'll try to review designs to reduce duplicaitonMotivation
At the moment we are testing in isolation: sources, wrapper. This approach misses the behavioral tests, when multiple sources with wrappers(processors,post-processors, deduplicates are enabled)
Unit tests for sources are scattered and require significant boilerplate to set up complex scenarios like headless services with pods and endpoint slices or multiple sources behaviour + wrappers to modify behaviour. A declarative approach simplifies adding new test cases.
Follow-up
More
Use cases (in theory should help to prevent regressions)
--min-ttloption #6014--service-type-filter=ClusterIP+ headless service withexternal-dns.alpha.kubernetes.io/endpoints-type=NodeExternalIPdoes not produce records #5867 (comment)