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.

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

tests: Introduce test/run script for Cockpit #12368

Merged
merged 1 commit into from
Jul 22, 2019

Conversation

marusak
Copy link
Member

@marusak marusak commented Jul 19, 2019

External projects use this script to run tests. Let's do the same in
Cockpit so tests/invoke does not have to differentiate between cockpit
and external projects.

This is not yet used. It is going to be enabled a bit later on, see #12367

@marusak marusak added the no-test For doc/workflow changes, or experiments which don't need a full CI run, label Jul 19, 2019
@marusak marusak requested a review from martinpitt July 19, 2019 14:03
@marusak
Copy link
Member Author

marusak commented Jul 21, 2019

Since it became rather simple, I wrote it in shell in the end.
In the previous PR you commented that if TEST_OS is not defined I should default to DEFAULT_IMAGE. This is kinda tricky since it is defined in python script. Anyway, I don't think it is needed at all - it has to be defined otherwise a lot of other things won't work - for example even test/verify/run-tests counts on this being defined and with the new tests syntax (with which this only work) TEST_OS is required to be specifically stated in the test name.

@marusak marusak mentioned this pull request Jul 21, 2019
2 tasks
Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Nice, thanks! BTW, I think I was wrong about having to rebase other PRs against this -- test/run gets called after the automatic rebasing in tests-invoke, so once this lands, other PRs should always see this.

test/run Outdated
# arguments but with an appropriate $TEST_OS, and optionally $TEST_SCENARIO

# Prepare image
if [[ $TEST_SCENARIO == container* ]]; then
Copy link
Member

@martinpitt martinpitt Jul 22, 2019

Choose a reason for hiding this comment

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

This is a bashism. Either use #!/bin/bash or use POSIX shell ([ "${TEST_SCENARIO#container}" != "$TEST_SCENARIO" ]). But let's just move these into the case loop where you already pattern-match the scenario. Then you have all necessary commands (prepare and run) in one place, which is also easier to follow. Then $TEST_SCENARIO will also always have a value, so that set -u will work.

test/run Outdated
@@ -0,0 +1,32 @@
#!/bin/sh -e
Copy link
Member

Choose a reason for hiding this comment

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

Please drop the -e here and add set -eu instead. That way, if you try to call this script like bash test/run for some reason, the options still apply. (Not that relevant here, but it's a good pattern to get used to).

The -u will also assert our assumptions about $TEST_OS always being set.

;;
container-*)
test/containers/run-tests --container ${TEST_SCENARIO#*-}
;;
Copy link
Member

Choose a reason for hiding this comment

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

Please add a *) default here with an error message.

External projects use this script to run tests. Let's do the same in
Cockpit so `tests/invoke` does not have to differentiate between cockpit
and external projects.

This counts with a new tests syntax where context is `image[/scenario]`.

Closes cockpit-project#12368
@marusak
Copy link
Member Author

marusak commented Jul 22, 2019

Thanks @martinpitt for the review, fixed. Also we could drop the kubernetes all together from master, right? But maybe as a separate commit so it is easier to pick to stable branches?

Copy link
Member

@martinpitt martinpitt left a comment

Choose a reason for hiding this comment

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

Nice, thank you!

Yes, let's drop the container-kubernetes test separately. I'll cherry-pick this PR into stable branches.

@martinpitt
Copy link
Member

I tested this with TEST_SCENARIO=avocado TEST_OS=fedora-30 test/run and it works nicely.

@martinpitt martinpitt merged commit ab1a3ac into cockpit-project:master Jul 22, 2019
mvollmer pushed a commit to mvollmer/cockpit that referenced this pull request Aug 1, 2019
External projects use this script to run tests. Let's do the same in
Cockpit so `tests/invoke` does not have to differentiate between cockpit
and external projects.

This counts with a new tests syntax where context is `image[/scenario]`.

Closes cockpit-project#12368
martinpitt pushed a commit that referenced this pull request Aug 1, 2019
External projects use this script to run tests. Let's do the same in
Cockpit so `tests/invoke` does not have to differentiate between cockpit
and external projects.

This counts with a new tests syntax where context is `image[/scenario]`.

Closes #12368
martinpitt pushed a commit that referenced this pull request Aug 29, 2019
External projects use this script to run tests. Let's do the same in
Cockpit so `tests/invoke` does not have to differentiate between cockpit
and external projects.

This counts with a new tests syntax where context is `image[/scenario]`.

Closes #12368
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-test For doc/workflow changes, or experiments which don't need a full CI run,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants