Remove useless e2e sleep time

[lburgazzoli]

Description

The tests to validate the correctness of the operator when components
are disabled have been refactored to:

1. take into account all the component's deployments, not only the first
   one to avoid swallowing misbehavior
2. remove useless waiting time before checking the presence of
   component's deployments because within the introduction of the
   internal APIs, the object hierarchy now allow for a proper cleanup as
   all the object have a proper owner reference. The operator deletes
   the component API using foreground deletion policy hence once the top
   level component CR is removed, the all the owned objects should have
   been deleted too.

How Has This Been Tested?

Screenshot or short clip

Merge criteria

• You have read the contributors guide.
• Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
• Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

[ykaliuta]

can this common parts be factored out to some common place?

[lburgazzoli]

can this common parts be factored out to some common place?

Yep, working on some shared common test/helpers since there are many places that are pretty much copy paste

[lburgazzoli]

/test opendatahub-operator-e2e

[lburgazzoli]

/test opendatahub-operator-e2e

[ykaliuta]

can this common parts be factored out to some common place?

Yep, working on some shared common test/helpers since there are many places that are pretty much copy paste

But you want it first, right?

It brings inconstancy with the Deployments lookup above.

Also, I would appreciate if you mentioned why it's correct (how the consistency is guaranteed), like due to ownership rules / finalizer after component CR removal it's guaranteed that no subdeployments left.

[lburgazzoli]

can this common parts be factored out to some common place?

Yep, working on some shared common test/helpers since there are many places that are pretty much copy paste

But you want it first, right?

It would love to, but it is still an ongoing effort and will take some more time to result in a acceptable PR :) . At this stage is more an exploration and there are quite a few things I'd like to clean up.

My motivation for this PR is to remove that useless 20 seconds delay per component e2e, hence cutting 140 second from the overall e2e test execution.

It brings inconstancy with the Deployments lookup above.

True.

I'm honestly not very sure about what is the intent of that check within the current shape of the operator, probably just a port of the old e2e tests were we didn't have a good object hierarchy, so checking the deployment was probably the only way to detect if a component was there or not. I probably could have done it without changing the deployment lookup, however it seems to be better to validate that all the deployments were removed instead of just one, which could swallow other issues.

Let me know if you want me to revert this part and leave it for a future PR.
No strong opinions

Also, I would appreciate if you mentioned why it's correct (how the consistency is guaranteed), like due to ownership rules / finalizer after component CR removal it's guaranteed that no subdeployments left.

As part of the commit or in the code ?

[ykaliuta]

can this common parts be factored out to some common place?

Yep, working on some shared common test/helpers since there are many places that are pretty much copy paste

But you want it first, right?

It would love to, but it is still an ongoing effort and will take some more time to result in a acceptable PR :) . At this stage is more an exploration and there are quite a few things I'd like to clean up.

My bad in wording. "it" I meant "this PR".

My motivation for this PR is to remove that useless 20 seconds delay per component e2e, hence cutting 140 second from the overall e2e test execution.

Totally understandable. In my old rework try I at lest replaced all such wait "loops" with conditional wait.

Was per-component check done in parallel?

It brings inconstancy with the Deployments lookup above.

True.

I'm honestly not very sure about what is the intent of that check within the current shape of the operator, probably just a port of the old e2e tests were we didn't have a good object hierarchy, so checking the deployment was probably the only way to detect if a component was there or not.

I think so. But in the current shape it still makes sense from e2e prospective since it rechecks that the ownership chain is correct and whatever is planned working as expected.

I probably could have done it without changing the deployment lookup, however it seems to be better to validate that all the deployments were removed instead of just one, which could swallow other issues.

Let me know if you want me to revert this part and leave it for a future PR. No strong opinions

Can both places be rewritten the same way?

Also, I would appreciate if you mentioned why it's correct (how the consistency is guaranteed), like due to ownership rules / finalizer after component CR removal it's guaranteed that no subdeployments left.

As part of the commit or in the code ?

Commit message.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@b36c13a). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1464   +/-   ##
=======================================
  Coverage        ?   19.11%           
=======================================
  Files           ?      158           
  Lines           ?    10358           
  Branches        ?        0           
=======================================
  Hits            ?     1980           
  Misses          ?     8154           
  Partials        ?      224           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lburgazzoli]

@ykaliuta

1. changed the tests to be consistent in relation to deployments lookup/validation
2. added some explanation in the commit message
3. added a small utility to list deployments given a GVK

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

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:

• OWNERS [ykaliuta]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[lburgazzoli]

/test opendatahub-operator-e2e

[lburgazzoli]

/test opendatahub-operator-e2e