leftover item(s) from previous PR: fixed unit tests in helm and fluxv2 TestGetInstalledPackageResourceRefs, move more shared code into plugins/pkg #4094
Conversation
plugins that had to do with GetInstalledPackageResourceRefs()
gfichtenholt
modified the milestones:
Pluggable support for Flux v2,
Pluggable support for Helm,
Pluggable architecture and new API
…urceRefs inside the test so as not to pollute the rest of the file
gfichtenholt
changed the title
…y multiple plugins
so they can be used by multiple plugins
There was a problem hiding this comment.
Thanks for the DRY-ing up of the code Greg. That'll make it much easier in the future to improve the pagination as well as maintenance of the shared functionality between helm/flux, for example.
I'm less keen to re-use the test cases (see inline comment), but if you think it's important to keep all the existing test cases in the helm plugin, OK (I don't - I'd be happy to only test there what is not tested in the new pkg module and specific to that plugin's use).
Thanks!
| Namespace: "default", | ||
| }, | ||
| ResourceRefs: resourcerefs.TestCases1[0].ExpectedResourceRefs, | ||
| newTestCase := func(tc int, identifier string, response bool, code codes.Code) testCase { |
There was a problem hiding this comment.
It looks (so far) like newTestCase is a function to convert an existing test-case format to one that can be used here, but I think I only guess that because of the context you provided in the PR description. Looks like it'd be worth a small comment in the code here for future readers (including us :P ).
That said, I'm also unsure why we need to do this (and generally try to avoid inheriting test cases, as it can end up down the road being very difficult to maintain). That is, if you're now unit-testing the functionality in the extracted module, we shouldn't need to repeat the tests here to keep their original form, but rather, just be able to test here anything that is specific to this module?
There was a problem hiding this comment.
yeah, that took a while to get working. My idea was that having moved some of the functionality to pkg (i.e. ResourceRefsFromManifest call), I generally want to have unit test cases there that test that func is working as expected. So I took the unit tests you had written for GetInstalledPackageResourceRefs and re-wrote them so they only test the ResourceRefsFromManifest part. Now, that's wondeful, but the function itself is not used in isolation, there is a higher level module (helm plugin) that has an API (GetInstalledPackageResourceRefs) which calls ResourceRefsFromManifest() AMONG other things it does. There are a few other funcs being called in the scope of GetInstalledPackageResourceRefs.
A clumsy analogy to help see thing better:
- ResourceRefsFromManifest is a unit test scenario
- GetInstalledPackageResourceRefs is an integration test scenario
If I had removed the unit tests from this module, we'd loose the "integration test" functionality. So I did not want to lose the hard work that went into writing that unit test, and instead just re-used some of the data from the "base" test as I call it.
The other angle is (as you know) it takes a lot of data to set up ResourceRefsFromManifest call properly. Like 700 lines of code. And the unit test is doing just that. So, If I still want to have the "integration" test, do I try to re-use a whole bunch of work that was done somewhere else or copy it, etc. I chose the latter.
Basically, I didn't want to loose any of your hard work. This isn't set in stone. I am willing to reconsider this if you feel strongly about it and/or find a better/cleaner solution. Let me know
I will add a comment to that effect in the code to hopefully make things a little clearer
There was a problem hiding this comment.
Yep, I think that's fine for now. I'll take a look next time I'm needing to touch the tests... I'm all for not removing them and re-using setup code, just keen to avoid running the same tests, but it may not be possible. Will see. Thanks!
| // 2. IMHO, it would have been better to get most of the detail info, including the current version out of | ||
| // parsed chart YAML file, which this implementation chooses to ignore. | ||
| // I did consider using flux's implementation of AvailablePackageDetailFromChart but did not feel comfortable | ||
| // chaning helm plugin to use it before talking to @minelson |
There was a problem hiding this comment.
I have no problem with the helm plugin being updated to use your implementation (and adding that here), assuming it's a drop-in replacement. I haven't looked at the helm code to see if the above assumption was based on previous restrictions, but great if it's not needed.
There was a problem hiding this comment.
ok, cool. save that one as a separate exercise for another day now. I put a TODO there. I am in the middle of something completely different right now
I have no problem with the helm plugin being updated to use your implementation (and adding that here), assuming it's a drop-in replacement. I haven't looked at the helm code to see if the above assumption was based on previous restrictions, but great if it's not needed.
| testCases = append(testCases, TestCases1...) | ||
| testCases = append(testCases, TestCases2...) | ||
| testCases := []resourcerefstest.TestCase{} | ||
| testCases = append(testCases, resourcerefstest.TestCases1...) |
There was a problem hiding this comment.
Related to an earlier comment, my preference would be to just leave the tests here in this module (not re-using them in other places), and instead update other modules to only test their specific code in the unit tests there (given that this module tests a lot of the functionality already).
There was a problem hiding this comment.
see what you think about what I said earlier as far as the reasoning why I did that. In the end, I WILL do as you see fit.
I had a leftover item from a previous large PR. You see I moved some common code into plugins/pkg. In this case it was
func ResourceRefsFromManifest(m, pkgNamespace string) ([]*corev1.ResourceRef, error) {so that both helm and flux plugin may reference it. Unit tests turned out to be a little tricky, because:
So I refactored the test, such that (1) contains some basic scenarios that only need ResourceRefsFromManifest func to work and (2) refactored both plugin unit tests TestGetInstalledPackageResourceRefs so that they use scenarios from (1) but also that the Server.GetInstalledPackageResourceRefs function, the way you had it before.
The number of files in this PR keeps on increasing, and it may look intimidating but don't get intimidated. For the most part I tried to find some code duplicated in two or more plugins and move that code into plugins/pkg directory and change the original code to use it from there. That's all it is. No significant changes to the code itself were made. Just moved from one place to another