Skip to content

Comments

test(cli): skip CLI tests if there is an override#738

Merged
manusa merged 1 commit intocontainers:mainfrom
matzew:skip_tests_for_cli_with_overrides
Feb 5, 2026
Merged

test(cli): skip CLI tests if there is an override#738
manusa merged 1 commit intocontainers:mainfrom
matzew:skip_tests_for_cli_with_overrides

Conversation

@matzew
Copy link
Collaborator

@matzew matzew commented Feb 4, 2026

When doing toolset override (E.g. to core, helm) or changing to read-Only: true, some tests fail.

One of those is the one on the CLI tests. In #517 some tests were skipped (due to downstream) if there are some overrides.

I think this works to some degree. IMO regardless of what the actual downstream (compile time) override of defaults is, is that test for "disabled" parts should be still tested, since they can be enabled by users

Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
@matzew matzew requested a review from manusa February 4, 2026 18:09
@matzew
Copy link
Collaborator Author

matzew commented Feb 4, 2026

One options is on things like common_test.go do some assumption, like:

	s.Cfg.ReadOnly = false
	s.Cfg.Toolsets = []string{"core", "config", "helm"}

(in this particular example)

@Cali0707
Copy link
Collaborator

Cali0707 commented Feb 4, 2026

One options is on things like common_test.go do some assumption, like:

	s.Cfg.ReadOnly = false
	s.Cfg.Toolsets = []string{"core", "config", "helm"}

(in this particular example)

IMO this is a good solution for the unit tests - I think that relying on defaults (which may be overriden in forks) is going to cause pain. Better to be explicit with what we are testing.

Perhaps if we want to have unit tests verifying the "default" behaviour, we can separate those out into a separate file with the same skip that you provided here, along with a test_default_overrides.go that could contain downstream overriden default tests

@manusa
Copy link
Member

manusa commented Feb 5, 2026

IMO it's fine and good having tests that rely on defaults (or actually verify them).

These are tests that sometimes are there on purpose to ensure that the MCP server has a default behavior.

I also like to have them in the same test suite as the other, since they are grouped organically.

The skip logic (which is used in other tests too), is effective, but it's true that is becoming cumbersome.
We can try to find a simpler way to do this.
Options I can think of (Besides what Calum proposes):

  • extract test cases to a separate file xxx_defaults_test.go with a build tag
  • create a file with the constant assertions that can have overrides (similar to what the production code has but for tests and assertions) -Not sure if this is what Calum propses-
  • Base Suite struct that exposes a skip method

For now I guess we should merge this PR and proceed with the agreed refactor later.

@manusa manusa added this to the 0.1.0 milestone Feb 5, 2026
@manusa manusa changed the title Skip CLI tests if there is an override test(cli): skip CLI tests if there is an override Feb 5, 2026
Copy link
Member

@manusa manusa left a comment

Choose a reason for hiding this comment

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

LGTM, thx!

@manusa manusa merged commit 5b24fbf into containers:main Feb 5, 2026
7 checks passed
@matzew
Copy link
Collaborator Author

matzew commented Feb 5, 2026

The other option would be to have some (config) bundles and pass them down to the test sets., e. like the xxx_defaults_test.go - Options:

  • default (as in upstream)
  • custom (e.g. what downstream might want), to be maintained by downstream

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