Skip to content

USHIFT-1119: Add load-balancer Robot Framework test#1708

Merged
openshift-merge-robot merged 9 commits intoopenshift:mainfrom
pacevedom:USHIFT-1119
May 5, 2023
Merged

USHIFT-1119: Add load-balancer Robot Framework test#1708
openshift-merge-robot merged 9 commits intoopenshift:mainfrom
pacevedom:USHIFT-1119

Conversation

@pacevedom
Copy link
Copy Markdown
Contributor

Which issue(s) this PR addresses:

Closes #

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 25, 2023
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Apr 25, 2023

@pacevedom: This pull request references USHIFT-1119 which is a valid jira issue.

Details

In response to this:

Which issue(s) this PR addresses:

Closes #

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.

@openshift-ci openshift-ci Bot requested review from dhellmann and ggiguash April 25, 2023 09:01
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 25, 2023
@pacevedom
Copy link
Copy Markdown
Contributor Author

/hold until CI jobs ack

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 25, 2023
Comment thread .gitignore Outdated
Comment thread test/Makefile Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

How can I run just 1 test file locally, if that's what I want?

This Makefile might make more sense as a shell script.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just supply the file, it is autocompleted because of the $(tests) target.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wasn't able to get that to work. I wrote a shell script in #1735

Comment thread test/resources/common.resource Outdated
Comment thread test/resources/kubeconfig.resource Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This 2 step pattern seems like something we're going to use a lot. Is there no Get Temporary Filename keyword? Maybe we should add one to the common resources.

Comment thread test/resources/kubeconfig.resource Outdated
Comment thread test/suites/load-balancer.robot Outdated
Comment thread test/suites/load-balancer.robot Outdated
Copy link
Copy Markdown
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

/lgtm

Comment thread test/resources/common.resource Outdated
Comment thread test/suites/load-balancer.robot Outdated
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2023
Comment thread test/Makefile Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wasn't able to get that to work. I wrote a shell script in #1735

Comment thread test/suites/load-balancer.robot Outdated
Comment thread test/suites/load-balancer.robot Outdated
Comment thread test/resources/kubeconfig.resource Outdated
Comment thread test/Makefile Outdated
Comment thread test/suites/load-balancer.robot Outdated
@openshift-ci openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label May 5, 2023
Copy link
Copy Markdown
Contributor

@dhellmann dhellmann left a comment

Choose a reason for hiding this comment

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

I'm happy with this version.

@jogeo had some comments on the previous draft, so I'll leave it for him to lgtm when he is ready.

Resource ../resources/kubeconfig.resource

*** Keywords ***
Setup Suite
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd like to discuss a "style guide" rule that the suite setup keywords for a test suite be in the file for that test suite, and reuse composable parts. That makes it more obvious what the preconditions are for a test when someone is working on it. Of course, the trade off is that if we need to change the startup logic we have to touch more files, but I'm not sure how often that is likely to happen. I think of it as composition vs. inheritance.

Don't change anything in this PR, but maybe we can talk about it as we evolve the early versions of the tests.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Was using this PR as some sort of guinea pig to test the CI job. Now that I got that running (meaning junit and such) it contains the bare minimum to run at least one test.

I expect the resource files to undergo massive changes as more tests are added. These are only placeholders to get things going while we acquire more knowledge about the framework.
I am all for composition, so the more primitive-like these files look, the better. We will see if they evolve into more complex functions once we add more tests (it might be that almost all setup/teardown strategies end up being the same).

I would be happy to merge this and then keep modifying it with new test suites.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Absolutely, yeah.

@jogeo
Copy link
Copy Markdown
Contributor

jogeo commented May 5, 2023

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2023
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, jogeo, pacevedom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [dhellmann,pacevedom]

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

@jogeo
Copy link
Copy Markdown
Contributor

jogeo commented May 5, 2023

/retest

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented May 5, 2023

@pacevedom: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

@jogeo
Copy link
Copy Markdown
Contributor

jogeo commented May 5, 2023

/hold cancel

@openshift-ci openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2023
@openshift-merge-robot openshift-merge-robot merged commit f4d833e into openshift:main May 5, 2023
@pacevedom pacevedom deleted the USHIFT-1119 branch December 18, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants