test: Add artifact plugin unit tests. re: argoproj#9812 #38
Conversation
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
|
cursor review |
|
Skipping Bugbot: Bugbot is disabled for this repository |
| assert.NoError(t, err) | ||
| assert.NotNil(t, location.Plugin) | ||
| // Note: SetType creates a new empty instance, not copying the values | ||
| assert.Equal(t, ArtifactPluginName(""), location.Plugin.Name) |
There was a problem hiding this comment.
This feels like a test that proves we have a bug. I feel like we should fix the bug instead.
There was a problem hiding this comment.
I can make SetType copy the values. Would you like any other behavior along with that?
There was a problem hiding this comment.
Humm, OK. Let's not change SetType now I've looked properly.
I've read the code and don't understand why this function exists in the form it does, but it's doing what it's documented to do. I'd remove this test from here and move it to TestArtifactLocation_SetType in workflow_types_test.go.
It'd also be good to consider if any other tests in there want similar extending.
There was a problem hiding this comment.
I was expecting this to be part of the test above it in the moved location, rather than a separate test. All the other artifact drivers are tested in the same way, so making this one different implies there is a difference; but there isn't.
| } | ||
|
|
||
| func TestArtifactLocation_MultiplePluginTypes(t *testing.T) { | ||
| t.Run("PluginTakesPrecedence", func(t *testing.T) { |
There was a problem hiding this comment.
Plugin doesn't seem to take precedence, this is proving that S3 takes precedence.
My preferred fix would be that configuring more than one artifact location triggers an error.
There was a problem hiding this comment.
I can update this Get method to the order that you like: https://github.com/pipekit/argo-workflows/blob/artifact-plugins/pkg/apis/workflow/v1alpha1/workflow_types.go#L1259
What is your preferred order?
There was a problem hiding this comment.
I'm not advocating for a specific order here.
The test is called "PluginTakesPrecedence", yet it proves that "S3TakesPrecedence".
My preference is that two artifact drivers is a misconfiguration and should be called out by erroring, and therefore precedence doesn't matter. Actually doing this is really out of scope. I'd suggest remove this test as it's testing for something we don't care about (precedence)
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Fixes #TODO Please do not open a pull request until you have checked ALL of these: * [ ] Create the PR as draft . * [ ] Run `make pre-commit -B` to fix codegen and lint problems. * [ ] Sign-off your commits (otherwise the DCO check will fail). * [ ] Use [a conventional commit message](https://www.conventionalcommits.org/en/v1.0.0/) (otherwise the commit message check will fail). * [ ] "Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue). * [ ] Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots. * [ ] Github checks are green. * [ ] Once required tests have passed, mark your PR "Ready for review". If changes were requested, and you've made them, dismiss the review to get it reviewed again. --------- Signed-off-by: J.P. Zivalich <j.p.zivalich@gmail.com>
Fixes #TODO
Please do not open a pull request until you have checked ALL of these:
make pre-commit -Bto fix codegen and lint problems.If changes were requested, and you've made them, dismiss the review to get it reviewed again.