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

run BATS tests in Cirrus #2947

Merged
merged 1 commit into from
Jun 17, 2019
Merged

run BATS tests in Cirrus #2947

merged 1 commit into from
Jun 17, 2019

Conversation

edsantiago
Copy link
Member

I'm running the BATS tests manually once in a while, and
catching several problems each week that make it past
the rest of CI. Since the BATS tests run at RPM gating
time, we need to catch problems earlier. Try running
the tests from Cirrus.

Signed-off-by: Ed Santiago [email protected]

@edsantiago
Copy link
Member Author

This PR is, at this moment, expected to fail CI due to #2899. I will need to rebase and re-test once that bug is fixed, but am submitting now to make sure that my addition works and causes Cirrus to fail.

@edsantiago
Copy link
Member Author

Guess this needs a lot more work: 'jq' is not installed on the test systems; Ubuntu has a too-ancient version of timeout; and I think I must be misunderstanding the make install because I'm seeing failures I wouldn't expect in newer podman. Back to the drawing board.

@edsantiago edsantiago force-pushed the bats_in_cirrus branch 2 times, most recently from cabd24c to 506a9ff Compare April 16, 2019 19:56
@rh-atomic-bot
Copy link
Collaborator

☔ The latest upstream changes (presumably #2991) 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 Apr 26, 2019
@openshift-ci-robot openshift-ci-robot added size/S and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M labels May 23, 2019
@edsantiago
Copy link
Member Author

CI failure is in ginkgo, in code I never touched. I can't figure out how to troubleshoot it.

@mheon
Copy link
Member

mheon commented May 23, 2019

Your tests are green

@edsantiago
Copy link
Member Author

@cevich could you PTAL at your convenience, and make sure this fits in with the CI framework? TIA.

cevich
cevich reviewed May 28, 2019
View reviewed changes
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.

My preference would be to have make localsystem run in a separate (for example) systemtest_script (in .cirrus.yml). The main reason being, it keeps the output separated in the WebUI, making it easier to debug problems. It also results in separate files being created in the log-storage bucket, should we ever get around to doing any secondary processing of those.

The only other thing I'd ask is if you'd consider nuking the optional_testing_task. It's purpose was originally to optionally run the system tests. But if you'd prefer they always run, also having an "optional" bit might confuse someone. Do you agree, or want me to remove it separately, or would keeping it be useful in some other way?

@edsantiago edsantiago mentioned this pull request May 28, 2019
Merged
@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
edsantiago added a commit to edsantiago/libpod that referenced this pull request Jun 3, 2019
@edsantiago
cirrus: minor cleanup and refactoring
a47bb4d 
...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 <[email protected]>
@cevich
Copy link
Member

cevich commented Jun 3, 2019

/approve

@edsantiago
Copy link
Member Author

Don't bother. It's a mess. Basicalluy, two PRs snuck in that broke my tests. Trying to fix them now.

@edsantiago
Copy link
Member Author

(Those don't explain the integration_test failures, but I have to force a new push anyway, so IO'm just going to ignore those)

@cevich
Copy link
Member

cevich commented Jun 13, 2019

...but there are compile warnings now...is something changing the code between system and integration tests? Like a 'git reset' or somesuch? Okay maybe not, those warnings are on master too.

I'm still struck by how different the integration tests "look", maybe it's the missing -nodes 3? It might be time to get dirty-hands with hack/get_ci_vm.sh on this PR. We've found a number of nasty things before, by looking while the tests run.

I'll give it a try once you rebase and push.

@edsantiago
Copy link
Member Author

The -nodes 3 is back; I removed it as part of my last rebase.

Unfortunately master is currently broken (#3325 and one more that I'm filing) so there's no point in proceeding with this PR until those get fixed.

@edsantiago
Copy link
Member Author

This makes no sense. System test should be failing: one failure as root, three as nonroot. (They're running. I checked the logs.) This has been a very surreal week.

@cevich
Copy link
Member

cevich commented Jun 13, 2019

Welp, tests look green and changes LGTM. Plus tomorrow's Friday, which is traditionally when everything goes FUBAR anyway, so 🍷 and deal with things Monday?

@rhatdan
Copy link
Member

rhatdan commented Jun 14, 2019

Looks like the tests passed?

@edsantiago
Copy link
Member Author

@rhatdan yes, and I think it's because we have no f30 testing hosts. As soon as we do, #3325 and #3326 will bite us on nonroot.

But... those are getting good attention, and it's possible they may get fixed soon, so I'm tentatively OK with merging this one in now; but not without @cevich's and @mheon's blessings.

@cevich
Copy link
Member

cevich commented Jun 14, 2019

@edsantiago now you see why some of my PRs tend to grow so large and take so long 😄

I'm conflicted on merging this. On one hand a functional make localsystem will benefit development efforts beyond/outside of CI. On the other, we're sorely behind the curve and late going live with F30 testing. Speaking of which...I see yet another rebase is needed over there...

What if "activating" the system-tests in CI was commented out temporarily, then the comment-removed when the issues are fixed? Would that help get this merged or just make the overall situation more complicated?

@edsantiago
Copy link
Member Author

Rebased against master, which includes #3328. CI passing (other than an iptables flake which passed on rerun) and I expect them to pass once we get f30 images in CI. I'd love to get this merged!

@mheon
Copy link
Member

mheon commented Jun 17, 2019

LGTM
@baude @rhatdan @haircommander @jwhonce @vrothberg PTAL

@rhatdan
Copy link
Member

rhatdan commented Jun 17, 2019

/lgtm

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

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 17, 2019
@rh-atomic-bot
Copy link
Collaborator

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

@edsantiago
run BATS tests in Cirrus
d6f4362 
I'm running the BATS tests manually once in a while, and
catching several problems each week that make it past
the rest of CI. Since the BATS tests run at RPM gating
time, we need to catch problems earlier. Try running
the tests from Cirrus.

Tests will be skipped on Ubuntu due to a too-ancient
version of coreutils (8.28; the 'timeout -v' we use
requires 8.29).

Tests are run *after* integration tests, even though
these take three minutes and would be nice to have
fail quickly, because running before causes bizarre
CI failures. Shrug.

UPDATE: also fix run test, broken by containers#3311.

Signed-off-by: Ed Santiago <[email protected]>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2019
@edsantiago
Copy link
Member Author

Rebased against master, fixed (simple) conflicts. Tests green again. Could I ask for one more lgtm please?

@haircommander
Copy link
Collaborator

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 17, 2019
@openshift-merge-robot openshift-merge-robot merged commit e639a8f into containers:master Jun 17, 2019
@edsantiago edsantiago deleted the bats_in_cirrus branch July 4, 2019 22:04
@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.
Reviewers

@cevich cevich cevich left review comments

@mheon mheon mheon left review comments

@baude baude Awaiting requested review from baude

@rhatdan rhatdan Awaiting requested review from rhatdan

Assignees

@rhatdan rhatdan

@haircommander haircommander

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@edsantiago @rh-atomic-bot @mheon @cevich @rhatdan @openshift-ci-robot @haircommander @openshift-merge-robot