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.

Sign up for GitHub

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

Jump to bottom

[ST] Big refactor of Test Storage usage in ST #9569

Merged
merged 5 commits into from
Feb 2, 2024
Merged

[ST] Big refactor of Test Storage usage in ST #9569

merged 5 commits into from
Feb 2, 2024

Conversation

jankalinic
Copy link
Contributor

Type of change

  • Refactoring

Description

This PR should consist of all the planned changes for testStorage class and its usage throughout the STs. The main focus point is the replacement of redundant variables that were being set directly from test storage, now there are only those with specific meaning to them. This PR also includes the changes to initialization of the TestStorage, every test should get it via the extensionContext from StorageMap.

Checklist

  • Write tests
  • Make sure all tests pass
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally

@jankalinic
Copy link
Contributor Author

@strimzi-ci run tests --cluster-type=ocp --cluster-version=4.14 --install-type=bundle --profile=acceptance

@jankalinic jankalinic self-assigned this Jan 18, 2024
@jankalinic jankalinic added this to the 0.40.0 milestone Jan 18, 2024
@strimzi-ci
Copy link

▶️ Build started - check Jenkins for more info. ▶️

@strimzi-ci
Copy link

❌ Test Summary ❌

TEST_PROFILE: acceptance
GROUPS:
TEST_CASE:
TOTAL: 35
PASS: 34
FAIL: 1
SKIP: 0
BUILD_NUMBER: 3
OCP_VERSION: 4.14
BUILD_IMAGES: false
FIPS_ENABLED: false
PARALLEL_COUNT: 5
EXCLUDED_GROUPS: loadbalancer,nodeport,olm

❗ Test Failures ❗

  • io.strimzi.systemtest.bridge.HttpBridgeTlsST

Re-run command:
@strimzi-ci run tests --profile=acceptance --testcase=io.strimzi.systemtest.bridge.HttpBridgeTlsST

@jankalinic jankalinic marked this pull request as ready for review January 19, 2024 09:49
@jankalinic jankalinic requested review from see-quick, im-konge and henryZrncik and removed request for see-quick and im-konge January 19, 2024 09:49
@jankalinic jankalinic force-pushed the testStorageRef branch 2 times, most recently from bcdc029 to 92843e6 Compare January 19, 2024 15:38
henryZrncik
henryZrncik approved these changes Jan 19, 2024
View reviewed changes
Copy link
Contributor

@henryZrncik henryZrncik left a comment

Choose a reason for hiding this comment

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

yeah as Lukas mentioned, in the discussion we agreed to go with having all data from extensionContext or/and going with TestStorage, but apart from this I do believe that rest of refactor is as we spoke about so LGTM so far.
Also should we not run more than acceptance profile on changes in basically almost all of the test classes ?

@jankalinic
Copy link
Contributor Author

yeah as Lukas mentioned, in the discussion we agreed to go with having all data from extensionContext or/and going with TestStorage, but apart from this I do believe that rest of refactor is as we spoke about so LGTM so far. Also should we not run more than acceptance profile on changes in basically almost all of the test classes ?

Based on the character of this refactor, i took the acceptance testrun as a minimum pass indicator, you are correct as all tests should be run.

@im-konge
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

see-quick
see-quick approved these changes Jan 23, 2024
View reviewed changes
Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

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

LGTM, assuming that tests pass.

@im-konge
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member

/azp run upgrade

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

im-konge
im-konge reviewed Jan 24, 2024
View reviewed changes
Copy link
Member

@im-konge im-konge left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, few nits

systemtest/src/test/java/io/strimzi/systemtest/kafka/QuotasST.java Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/log/LogSettingST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/log/LogSettingST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/log/LogSettingST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/metrics/MetricsST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/metrics/MetricsST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/operators/ReconciliationST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/operators/ReconciliationST.java Outdated Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/operators/topic/ThrottlingQuotaST.java Show resolved Hide resolved
systemtest/src/test/java/io/strimzi/systemtest/rollingupdate/RollingUpdateST.java Show resolved Hide resolved
@im-konge
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jankalinic
Copy link
Contributor Author

The only issue in azp regression is with testKafkaConnectAndConnectorStatus => missing topic example-topic-name in connect, this isn't however related to the PR, as it does not change code which might result in such error. Re-tested locally several times and the topic is present.

@im-konge
Copy link
Member

/azp run upgrade

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member

im-konge commented Feb 1, 2024

/azp list

@azure-pipelines Azure Pipelines
Copy link

CI/CD Pipelines for this repository:

@im-konge
Copy link
Member

im-konge commented Feb 1, 2024

/azp run namespace-rbac-scope-regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

im-konge
im-konge approved these changes Feb 2, 2024
View reviewed changes
Copy link
Member

@im-konge im-konge left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@jankalinic
Test Storage refactor
cf3f46c 
Signed-off-by: jkalinic <[email protected]>
@jankalinic
comment fix
676f3c3 
Signed-off-by: jkalinic <[email protected]>
@jankalinic
fix mistakes - failed tests
acb39ce 
Signed-off-by: jkalinic <[email protected]>
@jankalinic
fix keycloak secret
b045a20 
Signed-off-by: jkalinic <[email protected]>
@jankalinic
fix keycloak secret
75e59f1 
Signed-off-by: jkalinic <[email protected]>
@im-konge
Copy link
Member

im-konge commented Feb 2, 2024

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge im-konge merged commit c10257a into strimzi:main Feb 2, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@see-quick see-quick see-quick approved these changes

@henryZrncik henryZrncik henryZrncik approved these changes

@im-konge im-konge im-konge approved these changes

Assignees

@jankalinic jankalinic

Labels
System tests
Projects
None yet
Milestone
0.40.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jankalinic @strimzi-ci @im-konge @see-quick @henryZrncik