Skip to content
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

PipelineRollout Tests #41

Merged
merged 14 commits into from
Jun 7, 2024
Merged

PipelineRollout Tests #41

merged 14 commits into from
Jun 7, 2024

Conversation

afugazzotto
Copy link
Contributor

Fixes #20

Modifications

Updated the suite_test.go file to be able to setup an environment to test PipelineRollout reconciliation logic using Numaflow CRDs. Also, added test cases to pipelinerollout_controller_test.go file to test create, update, and delete of a PipelineRollout resource.

Verification

Issued make test command and verify all tests passed.

@afugazzotto afugazzotto changed the title Rollout tests PipelineRollout Tests Jun 5, 2024
@afugazzotto afugazzotto force-pushed the rolloutTests branch 2 times, most recently from ae0dbe8 to 3fb019e Compare June 5, 2024 23:10
By("bootstrapping test environment")
// TODO: IDEA: could set useExistingCluster via an env var to be able to reuse some tests cases in e2e tests
// by setting this variable in CI for unit tests and e2e tests appropriately.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does it mean "to reuse some test cases in e2e tests"? do you have an example of what you mean?

Copy link
Contributor Author

@afugazzotto afugazzotto Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test cases in the pipelinerollout_controller_test.go file test PipelineRollout lifecycle (create, update, delete operations). These could be reused in e2e tests and, when using a real/preexisting cluster, also the deletion of the Numaflow Pipeline on deletion of PipelineRollout could be tested (https://github.com/numaproj/numaplane/pull/41/files#diff-31f7e2289e4cc3f50a723cc05922642d3cc20247ed71f66dc7dab1a73d3f302cR237-R245).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. Possibly. Although, that will likely use the same test framework "github.com/stretchr/testify/suite" we were previously using....so you'd have to see if it would be easily callable from there, or otherwise may not be worth it

@juliev0
Copy link
Collaborator

juliev0 commented Jun 6, 2024

This is very elegant....but I'm not sure if I see any cases of things going wrong? should that be added to a subsequent PR? (at least the case of invalid yaml, not sure what else off the top of my head)

@juliev0
Copy link
Collaborator

juliev0 commented Jun 6, 2024

It should be noted this unit test doesn't really cover things on an individual function level, but maybe this is really the only good way to test Reconcile(). But it makes me wonder if we want to include individual function tests for some functions that are strictly related to data processing now or in the future. Any functions that are trivial it's probably not worth doing, but I'm not sure if any of our functions are non-trivial.

Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
@xdevxy
Copy link
Contributor

xdevxy commented Jun 6, 2024

@juliev0 do you want to take another look? Otherwise I will merge this. Thanks!

@juliev0
Copy link
Collaborator

juliev0 commented Jun 7, 2024

@juliev0 do you want to take another look? Otherwise I will merge this. Thanks!

please do, thanks!

@afugazzotto afugazzotto merged commit b4fe036 into main Jun 7, 2024
4 checks passed
@afugazzotto afugazzotto deleted the rolloutTests branch June 7, 2024 16:39
darshansimha referenced this pull request in darshansimha/numaplane Jun 10, 2024
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: dsimha <[email protected]>
darshansimha referenced this pull request in darshansimha/numaplane Jun 11, 2024
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: dsimha <[email protected]>
darshansimha referenced this pull request in darshansimha/numaplane Jun 25, 2024
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: dsimha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure create, update and delete logic works for PipelineRollout with proper unit tests
3 participants