Skip to content

Add "internal" external tests#1273

Closed
cgwalters wants to merge 1 commit intocoreos:masterfrom
cgwalters:nonext-tests
Closed

Add "internal" external tests#1273
cgwalters wants to merge 1 commit intocoreos:masterfrom
cgwalters:nonext-tests

Conversation

@cgwalters
Copy link
Member

The recently introduced "external" tests are written in a totally
different model a directory with a binary, an Ignition config etc.,
no Go code using kola/mantle required.
This is necessary because we don't want tests that live in a
separate Git repository to need to vendor the mantle/ Go code
or be too tied to it, etc.

However, in many cases the ergonomics of writing tests this way
are better than doing a lot of:

mustSSH("foo bar")
mustSSH("other stuff")

etc.

Let's hence add support for "internal" external tests that
come with coreos-assembler, so anyone writing a test can
choose which model they use.

I know "internal" external tests is confusing so...
maybe we can call these "directory tests"? "Split-out tests"?

I converted one test to this framework as a demo.

Also required adding support for distros to kola.json.

Note going down this path moves things more towards
"coreos-assembler as mechanism", and we could consider
moving more tests into e.g. fedora-coreos-config or so.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters

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

@jlebon
Copy link
Member

jlebon commented Apr 2, 2020

I think this is a really interesting topic!

I agree overall tests make the most sense as part of the FCOS config. This addresses the classic code <--> test binding issue (which Fedora STI also aimed to address by having tests in dist-git). For FCOS specifically, one idea we've been thinking about is binding FCOS releases to cosa refs which incidentally would also bind tests if they live here too. But yeah, having tests sit alongside lockfiles feels indeed more "correct".

That said, at a high-level, tests fall roughly in two categories wrt this discussion: those that are scoped to the host under test itself, and those that require multiple artifacts or somehow need "external" support (and again, worth mentioning the Fedora STI here I think partially addresses this by having artifact "selectors" for different tests). The test-as-systemd-unit path works well for the former as host-level tests, but we have (and will likely have more of) the latter too (e.g. testiso, run-upgrade, etc...).

(BTW, WDYT about "host tests" as the name for the test-as-systemd-unit path? Then it'd be "external host tests" and "internal host tests".)

Which... thinking on this more, motivates the question whether all tests could live in the config repo. Though how do we separate out the harness from the test code itself? For host tests like coreos.ignition.systemd.enable-service that's easier to do, though for e.g. testiso or run-upgrade that's harder.

I think we could get pretty far if we settle on the cosa build as the test subject. Then all the harness needs to do really is to pass the cosa build to the test. So e.g. the FCOS config repo would have tests/kola/iso, tests/kola/upgrade, tests/kola/host, etc... And of course, specific upstream projects could have more tests too which act upon the whole cosa build, not just the host.

Anyway, probably should peel this into a separate tracker issue but I do think this is an important topic, and definitely worth discussing with others in higher-bandwidth.

Back to this PR then, some specific notes:

  • I don't see the test being run in the CI output here: https://jenkins-coreos-ci.apps.ci.centos.org/blue/organizations/jenkins/github-ci%2Fcoreos%2Fcoreos-assembler/detail/PR-1273/9/pipeline/39#step-44-log-5. It should show up there, no?
  • For the time being, I think it's confusing to have these converted tests be in $gitroot/tests/kola (one could imagine cosa tests there which run in FCOS/RHCOS that verify properties unique to CoreOS systems). If we go with "host tests", how about mantle/kola/host-tests/. Otherwise, maybe mantle/kola/external-tests?
  • Instead of kola.json and a README.md, how about a test.yaml which includes a description and the metadata info? (Small nit there: would be nice for the distro/basearch inclusion/exclusion to be proper arrays.) Not a blocker for this, though worth considering before we go converting many more.

@darkmuggle
Copy link
Contributor

I don't have strong feelings about this idea, other than one of the strengths of the current Kola model is that the tests are built-in; if you have the binary you can re-produce the test. I like the flexibility proposed but I'd like to see a fingerprint of the test. Before tests could be done and knowing the Kola version would tell you the tests. With external tests, it makes comparing the tests a bit more difficult.

And I do like @jlebon's suggestion that YAML is better.

If we're going to do this pattern, I think making all tests external over internal is better. I think I like this slightly better than the current state of things. The core benefit to this PR is that any script can be a test.

@cgwalters
Copy link
Member Author

If we're going to do this pattern, I think making all tests external over internal is better. I think I like this slightly better than the current state of things. The core benefit to this PR is that any script can be a test.

We have tests that are doing things not covered by this model, including the recent tang ones. And more generally one can't do multi-node tests with external tests as they're written.

Conceptually with external tests the "driver" is the script on a single node, which is much more natural when doing single node tests I think, but much less sense when doing multi-node.

There may be a way to blend things more but I think we're going to have these two testing models for a while.

The recently introduced "external" tests are written in a totally
different model a directory with a binary, an Ignition config etc.,
no Go code using kola/mantle required.
This is necessary because we don't want tests that live in a
separate Git repository to need to vendor the mantle/ Go code
or be too tied to it, etc.

However, in many cases the ergonomics of writing tests this way
are better than doing a lot of:

```
mustSSH("foo bar")
mustSSH("other stuff")
```
etc.

Let's hence add support for "internal" external tests that
come with coreos-assembler, so anyone writing a test can
choose which model they use.

I know "internal" external tests is confusing so...
maybe we can call these "directory tests"?  "Split-out tests"?

I converted one test to this framework as a demo.

Also required adding support for `distros` to `kola.json`.

Note going down this path moves things more towards
"coreos-assembler as mechanism", and we could consider
moving more tests into e.g. `fedora-coreos-config` or so.
@cgwalters
Copy link
Member Author

cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Jun 9, 2020
This is a successor to coreos#1273

First, this test is a perfect example of "sudo sh" in Go that
is just much shorter and more obvious written in shell (that
runs as root by default too).

The other important thing here is that by having "external tests"
as part of the cosa project itself, we get coverage for that
code as part of our CI.
cgwalters added a commit to cgwalters/coreos-assembler that referenced this pull request Jun 11, 2020
This is a successor to coreos#1273

Move this test into
coreos/fedora-coreos-config#468
since it's just clearly better as shell, and to increase coverage
of external tests.
openshift-merge-robot pushed a commit that referenced this pull request Jun 11, 2020
This is a successor to #1273

Move this test into
coreos/fedora-coreos-config#468
since it's just clearly better as shell, and to increase coverage
of external tests.
@cgwalters
Copy link
Member Author

This is obsolete.

@cgwalters cgwalters closed this Jul 2, 2020
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Oct 14, 2020
All else equal, we prefer test contributors to use the external test
model where tests can live alongside the OS definition over adding more
"native" kola tests.

See previous discussions about this in
coreos#1273.
jlebon added a commit to jlebon/coreos-assembler that referenced this pull request Oct 15, 2020
All else equal, we prefer test contributors to use the external test
model where tests can live alongside the OS definition over adding more
"native" kola tests.

See previous discussions about this in
coreos#1273.
openshift-merge-robot pushed a commit that referenced this pull request Oct 15, 2020
All else equal, we prefer test contributors to use the external test
model where tests can live alongside the OS definition over adding more
"native" kola tests.

See previous discussions about this in
#1273.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants