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

Fix Checkbox configuration value resolution and add Metabox scenarios to test it #439

Merged

Conversation

pieqq
Copy link
Collaborator

@pieqq pieqq commented May 3, 2023

Description

This PR provides several changes:

  1. Refactor [environment] section testing using Metabox scenarios
  2. Add Metabox scenarios to test [test selection] section of Checkbox config
  3. Fix Checkbox config resolution order, namely: handle config from launchers last, so it takes precedence over anything else.
  4. Update documentation based on resolution order source code.

Please check individual commit messages for more information.

Resolved issues

CHECKBOX-436

Tests

# Running the config-related scenarios using Checkbox local
$ metabox local-source-22.04.py --do-not-dispose --tag config --log INFO
13:51:09 | INFO     Config: No 'uri' element defined.
13:51:09 | INFO     Config: Setting 'uri' to '/home/pieq/dev/work/checkbox'.
13:51:09 | INFO     Including scenario tag(s): config
13:51:22 | INFO     Starting scenario: config.environment.CheckboxConfXDG
13:51:23 | SUCCESS  [local][('jammy',)] config.environment.CheckboxConfXDG scenario has passed.
13:51:32 | INFO     Starting scenario: config.environment.CheckboxConfLocalHome
13:51:33 | SUCCESS  [local][('jammy',)] config.environment.CheckboxConfLocalHome scenario has passed.
13:51:44 | INFO     Starting scenario: config.environment.CheckboxConfLauncher
13:51:45 | SUCCESS  [local][('jammy',)] config.environment.CheckboxConfLauncher scenario has passed.
13:52:00 | INFO     Starting scenario: config.environment.CheckboxConfLocalHomePrecedence
13:52:01 | SUCCESS  [local][('jammy',)] config.environment.CheckboxConfLocalHomePrecedence scenario has passed.
13:52:11 | INFO     Starting scenario: config.environment.CheckboxConfLocalResolutionOrder
13:52:12 | SUCCESS  [local][('jammy',)] config.environment.CheckboxConfLocalResolutionOrder scenario has passed.
13:52:22 | INFO     Starting scenario: config.test_selection.TestSelectionDefault
13:52:22 | SUCCESS  [local][('jammy',)] config.test_selection.TestSelectionDefault scenario has passed.
13:52:38 | INFO     Starting scenario: config.test_selection.TestSelectionForced
13:52:38 | SUCCESS  [local][('jammy',)] config.test_selection.TestSelectionForced scenario has passed.
13:52:52 | INFO     Starting scenario: config.test_selection.TestSelectionExcludedJob
13:52:53 | SUCCESS  [local][('jammy',)] config.test_selection.TestSelectionExcludedJob scenario has passed.
13:53:05 | INFO     Starting scenario: config.test_selection.LocalTestSelectionResolution
13:53:06 | SUCCESS  [local][('jammy',)] config.test_selection.LocalTestSelectionResolution scenario has passed.
--------------------------------------------------------------------------------
13:53:08 | SUCCESS  Ran 9 scenarios in 114.430s

# Running the config-related scenarios using Checkbox remote
$ metabox remote-source-22.04.py --do-not-dispose --tag config --log INFO
13:53:21 | INFO     Config: No 'uri' element defined.
13:53:21 | INFO     Config: Setting 'uri' to '/home/pieq/dev/work/checkbox'.
13:53:21 | INFO     Config: No 'uri' element defined.
13:53:21 | INFO     Config: Setting 'uri' to '/home/pieq/dev/work/checkbox'.
13:53:22 | INFO     Including scenario tag(s): config
13:53:39 | INFO     Starting scenario: config.environment.CheckboxConfXDG
13:53:42 | SUCCESS  [remote][('jammy', 'jammy')] config.environment.CheckboxConfXDG scenario has passed.
13:54:07 | INFO     Starting scenario: config.environment.CheckboxConfRemoteHome
13:54:09 | SUCCESS  [remote][('jammy', 'jammy')] config.environment.CheckboxConfRemoteHome scenario has passed.
13:54:34 | INFO     Starting scenario: config.environment.CheckboxConfLauncher
13:54:36 | SUCCESS  [remote][('jammy', 'jammy')] config.environment.CheckboxConfLauncher scenario has passed.
13:54:56 | INFO     Starting scenario: config.environment.CheckboxConfLauncherPrecedence
13:54:58 | SUCCESS  [remote][('jammy', 'jammy')] config.environment.CheckboxConfLauncherPrecedence scenario has passed.
13:55:20 | INFO     Starting scenario: config.environment.CheckboxConfRemoteServiceResolutionOrder
13:55:22 | SUCCESS  [remote][('jammy', 'jammy')] config.environment.CheckboxConfRemoteServiceResolutionOrder scenario has passed.
13:55:45 | INFO     Starting scenario: config.test_selection.TestSelectionDefault
13:55:46 | SUCCESS  [remote][('jammy', 'jammy')] config.test_selection.TestSelectionDefault scenario has passed.
13:56:09 | INFO     Starting scenario: config.test_selection.TestSelectionForced
13:56:10 | SUCCESS  [remote][('jammy', 'jammy')] config.test_selection.TestSelectionForced scenario has passed.
13:56:32 | INFO     Starting scenario: config.test_selection.TestSelectionExcludedJob
13:56:33 | SUCCESS  [remote][('jammy', 'jammy')] config.test_selection.TestSelectionExcludedJob scenario has passed.
13:57:00 | INFO     Starting scenario: config.test_selection.RemoteTestSelectionResolution
13:57:02 | SUCCESS  [remote][('jammy', 'jammy')] config.test_selection.RemoteTestSelectionResolution scenario has passed.
--------------------------------------------------------------------------------
13:57:13 | SUCCESS  Ran 9 scenarios in 226.775s

pieqq added 4 commits May 3, 2023 13:43
Launcher should always take precedence over anything else (config file
from /etc/xdg/ or ~/.config/). In order to achieve this, it has to be
added last in the configs list to be processed.
Checkbox configuration covers many different sections. In order to
accomodate testing all the different sections inside the same "tag"
("config"), refactoring the existing tests (covering the [environment]
section) into its own scenario module.
@pieqq pieqq requested review from yphus, kissiel and Hook25 May 3, 2023 12:07
@Hook25
Copy link
Collaborator

Hook25 commented May 5, 2023

I have found a corner case where this (seems) to not work.

Creating checkbox.conf in /etc/xdg with the following:

[test selection]
forced = yes

Checkbox remote seems to ignore the configuration. Is this related to what was changed here?

@@ -68,6 +67,9 @@ def load_configs(launcher_file=None):
config = expand_all(os.path.join(d, config_filename))
if os.path.exists(config):
configs.append(config)
# Add config from launcher last so it gets precedence over others
if launcher_file:
configs.append(launcher_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

For the record, the ConfigParser read() works this way:

It is possible to read several configurations into a single ConfigParser, where the most recently added configuration has the highest priority. Any conflicting keys are taken from the more recent configuration while the previously existing keys are retained.

Copy link
Contributor

@yphus yphus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the config test directory reorganization, it helps a lot to separate the various files involved to validate each section.

@yphus yphus merged commit d7c7100 into main May 9, 2023
@yphus yphus deleted the CHECKBOX-436/config-resolution-order-and-metabox-scenarios branch May 9, 2023 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants