Test suite doesn't offer a way to override the default image #959

debarshiray · 2021-12-16T23:34:25Z

We currently offer a configuration file to override the default image that's used for things like the create command. Some downstream distributors use this to significantly change the default image to the point that the test suite can't be run unless it uses the same default image that's specified in the configuration file.

As an aside, I am worried that the test suite seems to be growing it's own logic to resolve container and image names parallel to the actual Toolbox code itself. We should try to fix this, because the logic isn't trivial.

When running the test suite on RHEL, @olivergs was using a downstream patch to introduce some environment variables to override the defaults, but I don't see how it can possibly work with the state of the test suite in Toolbox 0.0.99.3.

I think that we need a proper upstream solution for this.

The text was updated successfully, but these errors were encountered:

debarshiray · 2021-12-16T23:43:21Z

For reference, here's the downstream patch from RHEL:

From ae6de45466f93cf441408421f1804033c1525c0b Mon Sep 17 00:00:00 2001
From: Oliver Gutierrez <ogutsua@gmail.com>
Date: Tue, 14 Sep 2021 13:41:17 +0100
Subject: [PATCH] Tests fixes for gating

---
 test/system/102-list.bats      |  8 +++----
 test/system/103-container.bats |  2 +-
 test/system/libs/helpers.bash  | 38 +++++++++++++++++++++++++++-------
 3 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/test/system/102-list.bats b/test/system/102-list.bats
index 1989409b1207..c6ddf15c34d1 100644
--- a/test/system/102-list.bats
+++ b/test/system/102-list.bats
@@ -62,14 +62,14 @@ teardown() {
   run $TOOLBOX list --images
 
   assert_success
-  assert_output --partial "$(get_system_id)-toolbox:$(get_system_version)"
+  assert_output --partial "$(get_default_image_name)"
   assert_output --partial "fedora-toolbox:32"
 
   # Check containers
   run $TOOLBOX list --containers
 
   assert_success
-  assert_output --partial "$(get_system_id)-toolbox-$(get_system_version)"
+  assert_output --partial "$(get_default_container_name)"
   assert_output --partial "non-default-one"
   assert_output --partial "non-default-two"
 
@@ -77,9 +77,9 @@ teardown() {
   run $TOOLBOX list
 
   assert_success
-  assert_output --partial "$(get_system_id)-toolbox:$(get_system_version)"
+  assert_output --partial "$(get_default_image_name)"
   assert_output --partial "fedora-toolbox:32"
-  assert_output --partial "$(get_system_id)-toolbox-$(get_system_version)"
+  assert_output --partial "$(get_default_container_name)"
   assert_output --partial "non-default-one"
   assert_output --partial "non-default-two"
 }
diff --git a/test/system/103-container.bats b/test/system/103-container.bats
index e026a9da7bd6..d1df3334c0ea 100644
--- a/test/system/103-container.bats
+++ b/test/system/103-container.bats
@@ -15,7 +15,7 @@ teardown() {
 
 
 @test "container: Check container starts without issues" {
-  readonly CONTAINER_NAME="$(get_system_id)-toolbox-$(get_system_version)"
+  readonly CONTAINER_NAME="$(get_default_container_name)"
 
   create_default_container
 
diff --git a/test/system/libs/helpers.bash b/test/system/libs/helpers.bash
index d728c0fc5f20..021936545c4a 100644
--- a/test/system/libs/helpers.bash
+++ b/test/system/libs/helpers.bash
@@ -300,6 +300,7 @@ function find_os_release() {
 
 # Returns the content of field ID in os-release
 function get_system_id() {
+  if [[ -z ${TOOLBOX_TEST_SYSTEM_ID} ]]; then
     local os_release
 
     os_release="$(find_os_release)"
@@ -310,21 +311,28 @@ function get_system_id() {
     fi
 
     echo $(awk -F= '/ID/ {print $2}' $os_release | head -n 1)
+  else
+    echo ${TOOLBOX_TEST_SYSTEM_ID}
+  fi
 }
 
 
 # Returns the content of field VERSION_ID in os-release
 function get_system_version() {
-    local os_release
+    if [[ -z ${TOOLBOX_TEST_VERSION_ID} ]]; then
+      local os_release
 
-    os_release="$(find_os_release)"
+      os_release="$(find_os_release)"
 
-    if [[ -z "$os_release" ]]; then
-        echo ""
-        return
-    fi
+      if [[ -z "$os_release" ]]; then
+          echo ""
+          return
+      fi
 
-    echo $(awk -F= '/VERSION_ID/ {print $2}' $os_release | head -n 1)
+      echo $(awk -F= '/VERSION_ID/ {print $2}' $os_release | head -n 1)
+    else
+      echo ${TOOLBOX_TEST_VERSION_ID}
+    fi
 }
 
 
@@ -334,3 +342,19 @@ function check_xdg_runtime_dir() {
         export XDG_RUNTIME_DIR="/run/user/${UID}"
     fi
 }
+
+function get_default_container_name() {
+  if [[ -z ${TOOLBOX_TEST_DEFAULT_CONTAINER_NAME} ]]; then
+    echo $(get_system_id)-toolbox-$(get_system_version)
+  else
+    echo ${TOOLBOX_TEST_DEFAULT_CONTAINER_NAME}
+  fi
+}
+
+function get_default_image_name() {
+  if [[ -z ${TOOLBOX_TEST_DEFAULT_IMAGE_NAME} ]]; then
+    echo $(get_system_id)-toolbox:$(get_system_version)
+  else
+    echo ${TOOLBOX_TEST_DEFAULT_IMAGE_NAME}
+  fi
+}
-- 
2.31.1

It introduces the TOOLBOX_TEST_* environment variables, but they are not enough to influence things like pull_default_image.

HarryMichal · 2022-03-21T22:17:23Z

I see two approaches here:

Short term - continue with the current approach and reach the desired flexibility.
Long term - ditch bats and embrace Go testing frameworks so we can reuse parts of Toolbx related (not just) to the container/image resolving.

For the v0.1 release I'd try the short solution because I'd like to get the release out soon.

For the short term solution, a way could be making the suite more configurable using env vars or a dedicated TOML parser could be used to read a configuration. The first option sounds better to me as the second one will increase the complexity even more and most likely even add an additional dependency that most likely won't be widely available.

debarshiray · 2022-03-23T20:24:41Z

Umm... I don't think this related to the test framework or changing it to something else.

The issue here is that the test suite has it's own logic to determine what the default distro, image and release are, that has diverged from the actual logic inside the toolbox(1) binary. eg., the test suite doesn't know how to read configuration files.

One way to solve it is to add a private command to toolbox(1), let's call it resolve, that will dump the default distro, image and release values in a machine readable form. The rest of the test suite can then use this command to query these values instead of using it's own logic. Note that we would have to very carefully test this resolve command first, so that bugs in Toolbx don't corrupt the validity of the tests.

I think I had mentioned this approach before in past conversations, but good to finally put it down in writing.

HarryMichal · 2022-03-23T22:50:00Z

I don't like the idea of adding a command into the main binary that is meant only for testing. Instead, let's create a separate, test-only binary that uses the same code as toolbox(1).

debarshiray · 2022-03-24T11:01:39Z

I don't like the idea of adding a command into the main binary
that is meant only for testing. Instead, let's create a separate,
test-only binary that uses the same code as toolbox(1).

Sure, sounds like a better idea, indeed. :)

debarshiray · 2022-05-05T18:15:10Z

I don't like the idea of adding a command into the main binary
that is meant only for testing. Instead, let's create a separate,
test-only binary that uses the same code as toolbox(1).

Sure, sounds like a better idea, indeed. :)

#840 has already introduced a command that's of no use to end users and should stay hidden.

Let's stick to that approach, and not create a separate binary. There's no real philosophical difference between a command for testing, and a command for generating shell completions.

These are different from init-container because it gets baked into the Toolbx containers and users can see it with a podman inspect --type container ... or when things break.

HarryMichal · 2022-05-05T18:28:07Z

As much as I like the separation of concerns the preliminary work I did (https://github.com/HarryMichal/toolbox/tree/test/system/better-generalization-and-configurability) has showed that the introduced binary would further unnecessarily complicate our setup. It's definitely more convenient to keep everything in a single binary. And the final binary size should not be of concern really, too.

Though maybe hiding these commands a bit more would be welcome. Instead of toolbox test something like toolbox __test.

debarshiray · 2022-05-05T22:28:31Z

Though maybe hiding these commands a bit more would be
welcome. Instead of toolbox test something like toolbox __test.

Yes, namespacing them with __ or some other prefix sounds like a good idea. Whatever we do, we should retain consistency across all such hidden commands.

Commit 20f4f68 added support for configuration files. If keys in the configuration file were set the system test suite would not honor them. This puts additional strain on distributors overriding the default settings. This solves the problem by creating a new hidden command in Toolbx that provides interface for getting some information Toolbx uses under-the-hood (e.g., the default container name, image). This commit does not change the way images are cached in the system test suite. In case a different image is needed, the tests still need to be patched. Fixes containers#959

Commit 20f4f68 added support for configuration files. If keys in the configuration file were set the system test suite would not honor them. This puts additional strain on distributors overriding the default settings. This solves the problem by creating a new hidden command in Toolbx that provides interface for getting some information Toolbx uses under-the-hood (e.g., the default container name, image). This commit does not change the way images are cached in the system test suite. In case a different image is needed, the tests still need to be patched. Fixes containers#959 containers#1050

Commit 20f4f68 added support for configuration files. If keys in the configuration file were set the system test suite would not honor them. This puts additional strain on distributors overriding the default settings. This solves the problem by creating a new hidden command in Toolbx that provides interface for getting some information Toolbx uses under-the-hood (e.g., the default container name, image). This commit does not change the way images are cached in the system test suite. In case an additional image is needed, the tests still need to be patched. Fixes containers#959 containers#1050

debarshiray added the 1. Bug Something isn't working label Dec 16, 2021

HarryMichal added this to the Release 0.1.0 milestone Dec 17, 2021

HarryMichal linked a pull request May 12, 2022 that will close this issue

src,test/system: Respect configuration file in system tests #1050

Open

HarryMichal mentioned this issue Sep 21, 2022

Consider building and shipping toolbx images using Quay.io #1019

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test suite doesn't offer a way to override the default image #959

Test suite doesn't offer a way to override the default image #959

debarshiray commented Dec 16, 2021

debarshiray commented Dec 16, 2021

HarryMichal commented Mar 21, 2022

debarshiray commented Mar 23, 2022

HarryMichal commented Mar 23, 2022

debarshiray commented Mar 24, 2022

debarshiray commented May 5, 2022

HarryMichal commented May 5, 2022

debarshiray commented May 5, 2022

Test suite doesn't offer a way to override the default image #959

Test suite doesn't offer a way to override the default image #959

Comments

debarshiray commented Dec 16, 2021

debarshiray commented Dec 16, 2021

HarryMichal commented Mar 21, 2022

debarshiray commented Mar 23, 2022

HarryMichal commented Mar 23, 2022

debarshiray commented Mar 24, 2022

debarshiray commented May 5, 2022

HarryMichal commented May 5, 2022

debarshiray commented May 5, 2022