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

e2e: overhaul Performance-Addon-Operator tests #590

Merged
merged 8 commits into from Jul 4, 2023
Merged

e2e: overhaul Performance-Addon-Operator tests #590

merged 8 commits into from Jul 4, 2023

Conversation

Tal-or
Copy link
Contributor

@Tal-or Tal-or commented Mar 27, 2023 •
edited

Bunch of small fixes, dead-code removal and error handling to the existing tests.
Should not affect the functionally and be completely harmless.

Signed-off-by: Talor Itzhak titzhak@redhat.com

@Tal-or
Copy link
Contributor Author

Tal-or commented Mar 29, 2023

/retest

2 similar comments
@Tal-or
Copy link
Contributor Author

Tal-or commented Mar 29, 2023

/retest

@Tal-or
Copy link
Contributor Author

Tal-or commented Apr 3, 2023

/retest

ffromani
ffromani reviewed Apr 4, 2023
View reviewed changes
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

lots of good ideas in there. Let's discuss a couple of suspicious things and some ppossible further improvements

test/e2e/performanceprofile/functests/1_performance/cpu_management.go Outdated Show resolved Hide resolved
test/e2e/performanceprofile/functests/1_performance/cpu_management.go Show resolved Hide resolved
test/e2e/performanceprofile/functests/3_performance_status/status.go Outdated Show resolved Hide resolved
test/e2e/performanceprofile/functests/3_performance_status/status.go Outdated Show resolved Hide resolved
test/e2e/performanceprofile/functests/5_latency_testing/5_latency_testing_suite_test.go Outdated Show resolved Hide resolved
test/e2e/performanceprofile/functests/1_performance/cpu_management.go Outdated Show resolved Hide resolved
test/e2e/performanceprofile/functests/1_performance/cpu_management.go Outdated Show resolved Hide resolved
@Tal-or
Copy link
Contributor Author

Tal-or commented May 3, 2023

/retest

ffromani
ffromani reviewed May 4, 2023
View reviewed changes
Copy link
Contributor

@ffromani ffromani left a comment

Choose a reason for hiding this comment

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

/approve

I like this cleanup, lot of good stuff in there. Some details to iron out and we're good.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 4, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, Tal-or

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2023
@Tal-or Tal-or force-pushed the ci_touch_up branch 2 times, most recently from 60aacfd to 8b0e243 Compare May 9, 2023 11:25
@Tal-or
Copy link
Contributor Author

Tal-or commented May 10, 2023

/retest

@Tal-or
Copy link
Contributor Author

Tal-or commented May 17, 2023

/cc @ffromani

@openshift-ci openshift-ci bot requested a review from ffromani May 17, 2023 10:24
@ffromani
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 17, 2023
@Tal-or
Copy link
Contributor Author

Tal-or commented Jun 4, 2023

/retest

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 19, 2023
@Tal-or
e2e: remove deprecated function calls
870d20a 
Harmless and would keep the existing behavior

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: add error handling
ac18c70 
Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: remove redundant parenthesis
3f70040 
Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: adding WithTimeout + WithPolling to Eventually
c0149be 
Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: remove redundant type conversion
ed12ef8 
Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: error messages should not be capitalized
09bf2ed 
Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: remove unused functions and variables
1a16f71 
Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@Tal-or
e2e: print the updated performance profile
f0a565e 
    At the beginning of the test, prints the performance profile.
    This helps understand the baseline from which the test starts and tracks
    future changes in the logs.

Signed-off-by: Talor Itzhak <titzhak@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 4, 2023
@ffromani
Copy link
Contributor

ffromani commented Jul 4, 2023

/lgtm

thanks, looks nice

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 4, 2023
@jlojosnegros
Copy link
Contributor

I would like to take the opportunity to mention this PR that could help us in the future to find these problems

/lgtm

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 72625c4 and 2 for PR HEAD f0a565e in total

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 4, 2023

@Tal-or: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-merge-robot openshift-merge-robot merged commit 287fdce into openshift:master Jul 4, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jlojosnegros jlojosnegros Awaiting requested review from jlojosnegros

@MarSik MarSik Awaiting requested review from MarSik

@ffromani ffromani Awaiting requested review from ffromani

Assignees

@jlojosnegros jlojosnegros

@ffromani ffromani

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Tal-or @ffromani @jlojosnegros @openshift-ci-robot @openshift-merge-robot