Add unit tests #1195
Add unit tests #1195
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @elevran. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
k8s-ci-robot
added
the
size/XL
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
| Put(string, Cloneable) | ||
| Get(string) (Cloneable, bool) | ||
| Keys() []string | ||
| Clone() *Attributes |
There was a problem hiding this comment.
not related to this PR -
do we want to support in the future also Delete operation?
There was a problem hiding this comment.
We could, mostly for completeness, but I don't currently see a use case to remove an attribute from an endpoint.
k8s-ci-robot
added
size/L
| dv, ok := got.(*dummy) | ||
| assert.True(t, ok, "expected value to be of type *dummy") | ||
| assert.Equal(t, "foo", dv.Text) |
There was a problem hiding this comment.
Prefer to use assert.Equal for basic types and leave cmp.Diff for slices, maps and complex structs.
Changed to cmp.Diff throughout
| assert.Len(t, keys, 2) | ||
|
|
||
| found := map[string]bool{} | ||
| for _, k := range keys { | ||
| found[k] = true | ||
| } | ||
|
|
||
| assert.True(t, found["x"]) | ||
| assert.True(t, found["y"]) |
There was a problem hiding this comment.
instead of these lines, use:
expected:= []string{"x", "y"} and cmp.Diff?
There was a problem hiding this comment.
this would need to assume that order is fixed and matches or that we sort the keys. Not sure about sync,Map but map.Keys() order is intentionally randomized.
I think it is more work for this use.
Changed to use assert.ElementsMatch which seems more aligned.
| var defaultDataSources = DataSourceRegistry{} | ||
|
|
||
| // DataSourceRegistry stores named data sources and makes them | ||
| // accessible to other subsystems in the inference gateway. | ||
| // DataSourceRegistry stores named data sources. | ||
| type DataSourceRegistry struct { | ||
| sources sync.Map | ||
| } |
There was a problem hiding this comment.
why do we actually need DataSourceRegistry struct?
what would be different if we define defaultDataSources = sync.Map and move logic from the struct functions to the public functions?
There was a problem hiding this comment.
it makes testing (and especially parallel tests) much easier since there's no global state that needs to be synchronized between tests as each can use its own registry struct if needed.
There was a problem hiding this comment.
@nirrozenbaum since tests moved to the datalayer package, I could make DataSourceRegistry private if you think that would make it clearer.
| assert.Equal(t, podinfo, clone) | ||
| assert.NotSame(t, podinfo, clone) | ||
| assert.Equal(t, podinfo.Labels, clone.Labels) | ||
| clone.Labels["env"] = "staging" |
There was a problem hiding this comment.
testing that field change in clone doesn't change the original item was already tested in a different place. do you expect every clonable to test it?
There was a problem hiding this comment.
in principle, that would be the prudent thing to do as long as implementation of Clone() is not automated.
Need to ensure that slices, maps, pointers are not copied as-is to the clone but their values are.
Signed-off-by: Etai Lev Ran <elevran@gmail.com>
Signed-off-by: Etai Lev Ran <elevran@gmail.com>
Signed-off-by: Etai Lev Ran <elevran@gmail.com>
elevran
force-pushed
the
datalayer_unit_test
branch
from
34dc3db to
5bc90f5
Compare
|
/lgtm Thanks! |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elevran, nirrozenbaum 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 |
k8s-ci-robot
added
the
approved
* added unit tests Signed-off-by: Etai Lev Ran <elevran@gmail.com> * lint fix Signed-off-by: Etai Lev Ran <elevran@gmail.com> * simplify tests by removing redundant and using Signed-off-by: Etai Lev Ran <elevran@gmail.com> * changed to use cmp.Diff where appropriate Signed-off-by: Etai Lev Ran <elevran@gmail.com> --------- Signed-off-by: Etai Lev Ran <elevran@gmail.com>
Add base unit tests to all files under the datalayer.