fix(e2e): do not test deletion configmap is true #964

zdtsw · 2024-04-10T16:25:33Z

only test negative case when set to false: resource should remain
if we test postivie case, we wont get prow pod log after csv is deleted

Description

e2e flow:

creation => no changes in PR
- create DSCI
- create more DSCI, should fail
- create DSC
- create more DSC should fail
- ....
test configmap case => change in PR , previous as positive case to delete all, now negative should not delete
- set to configmap but not enable deletion label
- namespace still exists (MR namespace wont get label)
test deletion case => move from 2nd test suite to the 3rd one
- check all components are enabled first
- delete DSC CR
- loop thro components: see their resources are all gone.
- delete DSCI CR

How Has This Been Tested?

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
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

- only test negative case when set to false: resource should remain - if we test postivie case, we wont get prow pod log after csv is deleted Signed-off-by: Wen Zhou <wenzhou@redhat.com>

ykaliuta · 2024-04-10T17:04:05Z

So, because prow cannot get log we are not checking now if triggering deletion by config map works, or?

tests/e2e/controller_setup_test.go

tests/e2e/dsc_cfmap_deletion_test.go

ykaliuta · 2024-04-10T17:28:09Z

tests/e2e/dsc_deletion_test.go

@@ -123,3 +133,24 @@ func (tc *testContext) testAllApplicationDeletion() error {
 	}
 	return err
 }
+
+func (tc *testContext) testDSCIDeletion() error {


Basically, it's not test but just cleanup, isn't it?

it is a similar one to testDSCDeletion
it was in the "ConfigMap deletion" test suite, now, moved into "deletion" test suite.
both two functions are (could be a misleading function name) looking for named DSC or DSCI CR and delete it.
based on the pre-condition, CR should exist in the cluster, so we do not expect error.
But if CR is not in the cluster (not found) or error during .Delete() , we fail test

i renamed function to testDeletionExistDSCI()
does it make more sense?

CR should exist in the cluster, so we do not expect error.

Ah, that is the main point. Ok. May be put a comment in the code.

added comments in function

zdtsw · 2024-04-11T06:24:16Z

So, because prow cannot get log we are not checking now if triggering deletion by config map works, or?

by testing Configmap deletion, deletes everything of ODH which happens before Prow gather all logs etc.
before this PR:

1. if all good, we cannot find logs of operator pod => guess wont be many care about this log
1. if failed in creation case , we cannot find logs of operator pod => this troublesome to debug error
1. if failed in deletion by configmap, we get logs afterwards => this could be useful log

to address 2), made this PR, to not use "correct" configmap => skip deletion ODH

but i was thinking, either totally take away this test on configmap, or still test configmap, but for a negative case which is not set the label to "true", and check if e.g namespaces are still there (can check all others as well, but not really necessary)

- rename function - remove comments lines Signed-off-by: Wen Zhou <wenzhou@redhat.com>

ykaliuta · 2024-04-11T07:22:59Z

So, because prow cannot get log we are not checking now if triggering deletion by config map works, or?

by testing Configmap deletion, deletes everything of ODH which happens before Prow gather all logs etc. before this PR:

if all good, we cannot find logs of operator pod => guess wont be many care about this log

if failed in creation case , we cannot find logs of operator pod => this troublesome to debug error

if failed in deletion by configmap, we get logs afterwards => this could be useful log

to address 2), made this PR, to not use "correct" configmap => skip deletion ODH

but i was thinking, either totally take away this test on configmap, or still test configmap, but for a negative case which is not set the label to "true", and check if e.g namespaces are still there (can check all others as well, but not really necessary)

Yes, I understand the intention and it definitely makes sense to get logs from creation failures.
So if you think it's ok to remove the test, fine for me.

openshift-ci · 2024-04-11T07:23:20Z

[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

jstourac · 2024-04-11T11:40:55Z

but i was thinking, either totally take away this test on configmap, or still test configmap, but for a negative case which is not set the label to "true", and check if e.g namespaces are still there (can check all others as well, but not really necessary)

+point for thinking about negative test

A question - is this deletion via ConfigMap tested anyhow in upstream tests or is this functionality completely deferred to the QE tests far below in downstream only?

Was another approach considered - e.g. running this test as a single one in a separate run? This wouldn't affect log gathering of results for other tests. I guess.

just my 2cents

zdtsw · 2024-04-11T12:39:40Z

but i was thinking, either totally take away this test on configmap, or still test configmap, but for a negative case which is not set the label to "true", and check if e.g namespaces are still there (can check all others as well, but not really necessary)

+point for thinking about negative test

A question - is this deletion via ConfigMap tested anyhow in upstream tests or is this functionality completely deferred to the QE tests far below in downstream only?

Was another approach considered - e.g. running this test as a single one in a separate run? This wouldn't affect log gathering of results for other tests. I guess.

just my 2cents

it was tested in our e2e when we just added it in 2.4
but at that time, we did not cleanup everything, including CSV etc, so we were still able to get operator pod logs after e2e run.
but in 2.6/2.7 it added more to be cleaned up upon uninstallation, then it started to have missing pod log problem.
resources like CSV are per cluster, our e2e setup a real cluster, even we have our test suite running in parallel, it is using the shared cluster resources, so it wont really help even we can "test as a single one in a separate run"

ykaliuta · 2024-04-11T12:45:37Z

Well, it can be totally separate workflows in two different clusters I guess.

…es#964) * fix(e2e): do not test deletion configmap is true - only test negative case when set to false: resource should remain - if we test postivie case, we wont get prow pod log after csv is deleted Signed-off-by: Wen Zhou <wenzhou@redhat.com> * update: code review - rename function - remove comments lines Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com>

fix(e2e): do not test deletion configmap is true

624cd53

- only test negative case when set to false: resource should remain - if we test postivie case, we wont get prow pod log after csv is deleted Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw requested a review from VaishnaviHire April 10, 2024 16:25

openshift-ci bot requested review from AjayJagan and cam-garrison April 10, 2024 16:25

zdtsw removed request for AjayJagan and cam-garrison April 10, 2024 16:25

ykaliuta reviewed Apr 10, 2024

View reviewed changes

zdtsw force-pushed the chore_e2e_cm branch from 32c9b8a to dbe9e8a Compare April 11, 2024 07:07

update: code review

ef40091

- rename function - remove comments lines Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw force-pushed the chore_e2e_cm branch from dbe9e8a to ef40091 Compare April 11, 2024 07:13

ykaliuta approved these changes Apr 11, 2024

View reviewed changes

openshift-ci bot assigned ykaliuta Apr 11, 2024

openshift-ci bot added the lgtm label Apr 11, 2024

openshift-ci bot added the approved label Apr 11, 2024

openshift-merge-bot bot merged commit 130992d into opendatahub-io:incubation Apr 11, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(e2e): do not test deletion configmap is true #964

fix(e2e): do not test deletion configmap is true #964

zdtsw commented Apr 10, 2024

ykaliuta commented Apr 10, 2024

ykaliuta Apr 10, 2024

zdtsw Apr 11, 2024

zdtsw Apr 11, 2024

ykaliuta Apr 11, 2024 •

edited

Loading

zdtsw Apr 11, 2024

zdtsw commented Apr 11, 2024

ykaliuta commented Apr 11, 2024

openshift-ci bot commented Apr 11, 2024

jstourac commented Apr 11, 2024

zdtsw commented Apr 11, 2024

ykaliuta commented Apr 11, 2024

fix(e2e): do not test deletion configmap is true #964

fix(e2e): do not test deletion configmap is true #964

Conversation

zdtsw commented Apr 10, 2024

Description

How Has This Been Tested?

Merge criteria:

ykaliuta commented Apr 10, 2024

ykaliuta Apr 10, 2024

Choose a reason for hiding this comment

zdtsw Apr 11, 2024

Choose a reason for hiding this comment

zdtsw Apr 11, 2024

Choose a reason for hiding this comment

ykaliuta Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

zdtsw Apr 11, 2024

Choose a reason for hiding this comment

zdtsw commented Apr 11, 2024

ykaliuta commented Apr 11, 2024

openshift-ci bot commented Apr 11, 2024

jstourac commented Apr 11, 2024

zdtsw commented Apr 11, 2024

ykaliuta commented Apr 11, 2024

ykaliuta Apr 11, 2024 •

edited

Loading