Skip to content

test: refactor and addition of CR deletion test#17

Merged
nbalacha merged 1 commit intoopenshift:mainfrom
leelavg:unit-test
Dec 15, 2021
Merged

test: refactor and addition of CR deletion test#17
nbalacha merged 1 commit intoopenshift:mainfrom
leelavg:unit-test

Conversation

@leelavg
Copy link
Copy Markdown
Contributor

@leelavg leelavg commented Dec 10, 2021

  • use envtest for verifying reconciliation of lvm cluster CR

Signed-off-by: Leela Venkaiah G lgangava@redhat.com

@jmolmo
Copy link
Copy Markdown
Contributor

jmolmo commented Dec 10, 2021

IMHO we are not testing the functionality of the controller. I mean we are just checking that controller runtime client works properly using a CRD. And that is something that has been already verified in the client package.

I think that it has more meaning and utility for us the test proposed in #13, where after the creation of the lvmcluster CRD we verified the existence of the CSIDriver resource and topolvm deployment.

In any case ... is difficult for me to see this kind of test as "unit test" ,,, is more like a mix between integration test (without CI env) and "extended" unit tests.

@leelavg
Copy link
Copy Markdown
Contributor Author

leelavg commented Dec 11, 2021

And that is something that has been already verified in the client package

  • It was verified for an example resource creation, in our case we want to test creation of our CR with default spec for now and check status is true & reconciliation started and working as expected

I think that it has more meaning and utility for us the test proposed in #13

  • the said pr will be split into three and this is the first one where I'm trying to create a layout for e2e-ish kind of flow and rest of 2 prs will build up on this layout

In any case ... is difficult for me to see this kind of test as "unit test" ,,, is more like a mix between integration test (without CI env) and "extended" unit tests.

  • yep, it's arguable, in a unit test-ish setup we have test spies, mocks/fakes etc however I don't want to stop moving forward to decide on how unit tests should look like at this point/pace of development 😅

IMHO, if we had flow charts then it suites better to commit failing tests first and then code to fix those tests

I'm open to suggestions on how good to refactor tests or what should be tested as part of unit tests and e2e tests.

Pls correct me if I presented something not relatable/right 😀

- use envtest for verifying reconciliation of lvm cluster CR

Signed-off-by: Leela Venkaiah G <lgangava@redhat.com>
@leelavg leelavg changed the title test: add lvm cluster CR deletion test test: refactor and addition of CR deletion test Dec 15, 2021
Copy link
Copy Markdown
Contributor

@jmolmo jmolmo left a comment

Choose a reason for hiding this comment

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

I wonder is we can use the structure of this test to do all the "unit test" for all the "functional units".
We need always to create the lvm cluster and start the reconcile loop, after that ,, the verification we need to do for each unit are basically the same ( the unit is created, deployed, and it is composed by several containers) .

@leelavg
Copy link
Copy Markdown
Contributor Author

leelavg commented Dec 15, 2021

@jmolmo yes, that's the plan.

The current pr creates a basic skeleton and individual component owners add their units with their implementation.

@nbalacha nbalacha merged commit b7cc3da into openshift:main Dec 15, 2021
@leelavg leelavg deleted the unit-test branch December 21, 2021 03:13
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.

3 participants