Skip to content

Conversation

@edsantiago
Copy link
Member

...with the goal of (very soon) reusing this code, in #2947,
to run system tests in CI. This is the cleanest way I can
think of to do so without duplication or a large maintenance
burden.

Changes are:

  • replace references to 'ginkgo' with 'integration'. That
    target is already in Makefile, and is not only more
    readable, it's also more abstract. There is no reason
    for this level of code to know about ginkgo.
  • replace a 'noxxx' option with the more usable 'xxx'
    • and fix a logic bug: the '-n' (no-remote) option
      should not be treated as an actual test-defining
      argument option.
  • allow rootless_test.sh to accept an argument,
    that being the name of the test suite to run
    (default: integration). run BATS tests in Cirrus #2947 will enable 'system'.
  • allow integration_test.sh to serve multiple purposes,
    by checking its filename. run BATS tests in Cirrus #2947 will add a symlink,
    system_test.sh, which will then cascade down to
    invoke system tests.

Signed-off-by: Ed Santiago santiago@redhat.com

@edsantiago
Copy link
Member Author

@cevich PTAL; this is intended as a prerequisite for the review feedback you gave on #2947.

@edsantiago edsantiago requested a review from cevich May 28, 2019 21:25
Copy link
Member

@cevich cevich left a comment

Choose a reason for hiding this comment

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

LGTM with one big-picture concern, if not fixed here, something that should be done in another PR. I do not see any end to growth in the number of testing-permutations - A likely next one will be STORAGE_DRIVER related. Something to consider, and I'm interested in your thoughts on fixing it.

@cevich
Copy link
Member

cevich commented May 30, 2019

LGTM

@baude @mheon wanna peek?

@edsantiago
Copy link
Member Author

This unfortunately hard-conflicts with @baude's #3236

@cevich
Copy link
Member

cevich commented May 31, 2019

This unfortunately hard-conflicts

Yeah, we've been needing some change in this area for a while. It's okay, code-conflicts just mean we should clunk our heads together until a more holistic solution presents.

The current design has most flows running through intergration_test.sh, all with slight permutations on the meaning of "Integration test" (and how it's run). There's a lot of duplication in Brent's old container_test.sh too (which was originally used to handle multiple different testing styles and setups).

As you noticed before from my comments, I'm a bit torn between the different approaches, I like both separate/simple scripts and grand-poo-bah --with --options.

Perhaps if we all "tat and sinkt bout it" together, with a drawring of our matrix, that would help?

@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #3236) made this pull request unmergeable. Please resolve the merge conflicts.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 1, 2019
...with the goal of (very soon) reusing this code, in containers#2947,
to run system tests in CI. This is the cleanest way I can
think of to do so without duplication or a large maintenance
burden.

Changes are:
 - replace references to 'ginkgo' with 'integration'. That
   target is already in Makefile, and is not only more
   readable, it's also more abstract. There is no reason
   for this level of code to know about ginkgo.
 - allow rootless_test.sh to accept an argument,
   that being the name of the test suite to run
   (default: integration). containers#2947 will enable 'system'.
 - allow integration_test.sh to serve multiple purposes,
   by checking its filename. containers#2947 will add a symlink,
   system_test.sh, which will then cascade down to
   invoke system tests.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2019
@edsantiago
Copy link
Member Author

Rebased

@cevich
Copy link
Member

cevich commented Jun 3, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 3, 2019
@baude
Copy link
Member

baude commented Jun 3, 2019

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: baude, edsantiago

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 3, 2019
@cevich
Copy link
Member

cevich commented Jun 3, 2019

/retest

@openshift-merge-robot openshift-merge-robot merged commit 0ede794 into containers:master Jun 3, 2019
@edsantiago edsantiago deleted the cirrus_cleanup branch June 3, 2019 19:56
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 26, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants