`integration` pytest mark we use in our test suite is a misnomer #949

Open Cynical-Optimist opened 4 years ago

See original issue on GitLab In GitLab by [Gitlab user @phildawson] on Mar 7, 2019, 09:52

Summary

The integration pytest mark we use in our test suite is a misnomer. In reality, tests with this mark are not necessarily integration tests, we use the mark to label tests which use a sandbox. It would be nice to replace this mark with one which better reflects its use.

Possible fixes

Replace integration pytestmark with sandboxed mark or similar.

Other relevant information

This was originally discussed in the comments on !1158

BuildStream version affected: /milestone %BuildStream_v1.x

In GitLab by [Gitlab user @phildawson] on Mar 7, 2019, 09:52

mentioned in merge request !1158

In GitLab by [Gitlab user @phildawson] on Mar 7, 2019, 09:54

changed the description

In GitLab by [Gitlab user @tlater] on Mar 8, 2019, 15:14

Note that the HAVE_SANDBOX variable also exists. It fulfills a similar purpose (ensuring that sandbox tests are not executed on certain systems), so perhaps the two concepts could be conflated. The HAVE_SANDBOX variable is probably a superset of the integration mark.

In GitLab by [Gitlab user @cs-shadow] on May 23, 2019, 15:16

mentioned in merge request !1353

In GitLab by [Gitlab user @willsalmon] on May 30, 2019, 10:56

There are some tests that need a sand box but are not covered by the integration marker.

That said the integration marker dose seem misleading a little

Some tests are testing how bst integrates with its sandbox's as they are separate tools. It dose make some seance.
But many of the integration tests are not testing if bst plays well with its sandbox's and are really testing things like elements behaviour.

In GitLab by [Gitlab user @tristanvb] on May 31, 2019, 07:04

The reason why we have separated tests under the integration marker to be run optionally is mainly:

So that you don't have to download a runtime to run tests unless you buy into that
So that for testing, you really only need the repository and nothing else assuming BuildStream is already installed (for example, you are running your tests while hacking on the plane, and/or for whatever do not have internet access at the time)
- This second bit may be regressed by tox, but that would be nice to fix (I think tox has a semantic which allows the tox.ini to set a directory for caching downloaded dependencies, so that it will be able to reconstruct the environment without reaching for the internet if it has at least created the environment once before and cached those deps).

We chose the word "integration" because downloading a runtime essentially meant integrating with an external component, this could very well be a misnomer, just wanted to point out the essential reasons why it is important to have this separation, as this might help shed some light in finding a better name for it.

In GitLab by [Gitlab user @tristanvb] on May 31, 2019, 07:11

I should add one more thing: I consider it a bug that the integration tests has a separate fixture or works in any way different from the regular tests.

There are cases in the tests which we don't want to repeat, so it is useful for tests to be able to share built artifacts - this could be an explicit feature to be declared by a test case rather than being implied by virtue of being an integration test
There are cases (most/all) where downloaded sources should be persisted in the INTEGRATION_CACHE (or whatever that ends up being renamed to) between test runs... that should also be explicit rather than being implied.
There is some weird notion that the integration tests should be munging this common makefile and have some similar testing strategy (historically this comes from the tests all comparing their results to some committed version of a checkout), this is patently untrue; any sort of test can qualify as an integration test as long as it must run in a sandbox with some downloaded runtime.

Over all, this could do well with a lot of untangling.

buildstream-migration / bst-staging