ts: close remaining Codecov gaps in tcl/, fix 3 dead branches - #672
Merged
Conversation
Two branches of tcl/init.tcl.in had no test reaching them: the env-var-override path of the color config init proc (a plain 'module' invocation never overrides MODULES_COLOR, only the default and command-line paths for that option were exercised) and the id-output-groups-not-last-field path of __initStateUsergroups (the fake 'id' used across the whole suite always prints 'groups=' as its last field, so the branch handling a trailing field placed after it by some systems, e.g. a security context, was never taken). Add an unconditional MODULES_COLOR override case to 140-color.exp (placed before its skip_if_quick_mode so it always runs), and append a trailing field to testsuite/id fake output -- verified this does not change the parsed group list, since the parser stops at the last space before the next '=' either way. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The startup logic in subcmd.tcl.in that reads the site-wide .modulespath file resolves each colon-separated path entry as a glob pattern, keeping the entry as a literal path when the glob matches nothing. The install example .modulespath.in fixture only ever listed entries that do resolve, so that branch, and the module use call appending the collected paths, were never exercised. Add a third, non-existent path entry to the fixture, and extend 120-autoinit.exp expectations for every scenario that sources it, so the resulting MODULEPATH and virtual init collection now include it unresolved. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The module-warn modulefile command (mfcmd.tcl) raises "Missing value for '--message' option" when --message is the last token in its argument list, with nothing left to consume as the value. The existing warn/.modulerc and warn/1 fixtures covered every other module-warn error case (no message, no module, unknown option) but not this one, since none of their scenarios ended the argument list right after a bare --message. Add a new scenario to both fixtures and a matching case to 700-module-warn.exp for each of the rc and modulefile call sites. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The array-key lookup helpers in modspec.tcl that back extended_default resolution (one variant used when implicit_default is on, one when it is off) resolve a partial version query matching several available versions by first checking whether the directory has an explicit default version and, only if none of the matches is that explicit default, falling back to the highest matching version. No existing fixture had a module name with two sibling versions, one of them set as default through a .version file, queried through a partial spec that matches both, so the explicit-default branch of either variant was never taken. Add a new fixture module with versions 1.0.1 and 1.0.2 where 1.0.1 is the explicit default, and a test loading it through the partial spec 1.0 with extended_default enabled, checked with implicit_default both on and off since that selects which of the two lookup variants runs. Update 140-hide-full-path.exp expected listing for the new fixture, which that test enumerates as part of the whole modulepath. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The module-spec argument parser in modspec.tcl, when a sub-command allows a spec with no module name, flushes a variant-only element still pending when a following element starts a name of its own by substituting a wildcard name for it. No existing avail test passed a variant-only argument ahead of a named one in the same command line, so that flush branch was never taken. Add such a case to 270-adv_version_spec.exp: a variant assignment argument followed by a separate named argument, checked against the plain named lookup so the wildcard element is confirmed to have no effect on the result. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
extensions-sc, the scan-time stand-in for the extensions modulefile command in modscan.tcl, records a provided-alias element alongside provide only when info_extension is off, the default for that option. The only existing test exercising a scanned extensions command first turned info_extension on, so the default-off case was never scanned. Add a case using the existing provide fixture ahead of that override, checking both the provide and provided-alias search results at the default setting. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The version-handling loop in getModules (tcl/modfind.tcl.in) has carried a dead branch since it was introduced. Two commits landed back to back on 2020-10-25 while fixing an issue about global/user rc definitions not showing up in results: the first made the loop add every such rc-defined version to the found list and the module symbol list unconditionally, dropping the previous gate on whether rc definitions were asked to be included; the second, right after it, added a further else-if branch meant to still record an rc definition in the module symbol list when that inclusion flag is false, apparently written without noticing the branch above it now already fires whenever a version has an rc definition at all, flag or not. Since the main condition already covers every case the else-if checks, the else-if can never run, and it never has, from the day it was added -- confirmed by checking out the state right before that second commit in git history. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The module-listing proc in tcl/report.tcl.in builds its JSON-mode
display list by aliasing display_list to clean_list via 'upvar 0',
then guarding with 'if {![info exists display_list]} { set
display_list {} }' in case clean_list was never populated. clean_list,
though, is unconditionally set to an empty list right at the top of
the same proc, before the per-module loop that may or may not append
to it -- so by the time the alias is created, the variable it points
to already exists, and the guard can never fire.
Checked out the state of modulecmd.tcl.in right before the commit
that introduced this upvar/guard pair (originally for a different,
now-refactored JSON-support commit) to confirm the same unconditional
pre-loop initialization was already present at that point too: the
guard has been dead from the day it was added, not from a later
refactor.
Drop the guard, keeping only the alias. This changes nothing at
runtime since the branch was unreachable, so no new test is needed.
Assisted-by: Claude:claude-sonnet-5
Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The array-key lookup helpers in modspec.tcl are backed by two alternative proc bodies, swapped in under a single common name by a definer proc depending on whether implicit_default is on. Its rename-the-previous-variant-back branch only runs on a second call with a different value than the first, and nothing in the normal command dispatch path ever calls it more than once per process -- its only caller runs exactly once per top-level command. A site configuration script, though, runs in the same interpreter with full access to every internal proc, and the suite already uses that access to cover the identical rename-back branch of three sibling helpers through the existing on-the-fly-procedure-redefinition case in siteconfig.tcl-1 -- the array-key lookup definer was simply missing from that coverage. Add a matching case: call the definer with implicit_default 0, then 1, then 0 again, reporting a lookup through the active variant after each call to confirm it still resolves correctly once swapped both ways. Verified against a probe copy of the built interpreter that the added middle and third calls do reach the rename-back branch (confirmed by a temporary print statement there, since a local coverage-instrumented run was out of scope here), and with 'TESTSUITE_ENABLE_SITECONFIG=1 script/mt 00/120' per the existing convention for exercising this siteconfig fixture locally (a real install is otherwise required, since the primary siteconfig option has no environment-variable override). Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The generic env-var/command-line override handling inside getConf (tcl/init.tcl.in) has two branches no current config option is shaped to reach: the single-element validvallist case of its validation switch, meant for a Tcl 'string is <class>' check rather than a fixed value list, since every option using an unkeyed validvallist today either has zero elements or two or more; and the generic intvallist conversion at the end of that same block, since the only option that sets intvallist, color, also sets an initproc that supersedes this whole path before reaching it. Both branches are reachable by giving an option that different shape at runtime, and the suite already does exactly that for other options through g_config_defs overrides in siteconfig.tcl-1 (see the existing bad-default-value cases for tag_abbrev and variant_shortcut, and the init_envvars case right below the new ones). Add two more: superseding run_quarantine with a single-element 'integer' validvallist and an overridden environment variable to exercise the string-is class check, and superseding csh_limit (which has no environment variable of its own to fake) with a two-element validvallist and intvallist to exercise the generic conversion on its plain default value. Verified with 'TESTSUITE_ENABLE_SITECONFIG=1 script/mt 00/120' per the existing convention for exercising this siteconfig fixture locally (a real install is otherwise required, since the primary siteconfig option has no environment-variable override), plus the same file without that variable set and the Tcl lint check, all matching pre-change results other than the two new passing cases. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
Corrected understanding of what Nagelfar/Codecov actually flag on a bare closing brace of an else-less if: it means the condition never evaluated false in any run, not that the block was never entered. Two consequences: The array-key lookup helper collecting matching tags checks "$tag ni $tag_list" to skip a tag root already recorded earlier in the same call, but that check can never observe a duplicate, since the proc that populates the underlying tag structure always deduplicates by tag name before appending -- so this skip branch is unreachable through module-tag/module-hide/module-forbid. Force the shape directly through the global tag structures from siteconfig.tcl-1 instead, the same way the suite already does for config option shapes: two identical tag entries at different indices, so the second lookup observes the tag already recorded by the first. The array-key definer redefinition case added earlier only ever called it with a value different from the one before, so it never exercised the no-op branch where the currently active variant already matches what is being asked for -- add a same-value call at the start, matching the shape the existing on-the-fly-procedure- redefinition case already uses for its own sibling helpers (same value twice, then different twice). Also add a case for the available-version-list lookup helper's unregistered-argument path: its only real caller always passes an argument already registered through the module-spec parser, so call it directly with one that was never registered. Verified with 'TESTSUITE_ENABLE_SITECONFIG=1 script/mt 00/120' (65 passes) plus the same file without that variable set and the Tcl lint check, not the full suite. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
The unmet-dependency cleanup loop in unsetModuleDependency checked "[info exists ::g_unmetDepHash($ummod)]" before touching that entry, but the two structures it reads from, g_moduleUnmetDep(mod) and g_unmetDepHash(ummod), are only ever written together, in the same statement pair in setModuleDependency (one lappend per unmet occurrence to each, always paired), and only ever cleared together, in this exact loop. Nothing else in the file touches either structure, so every module referenced in g_moduleUnmetDep(mod) is guaranteed to still have a corresponding g_unmetDepHash entry when this loop runs -- the guard can never evaluate false. Drop the guard, keeping its body unconditional. This changes nothing at runtime since the branch was unreachable, so no new test is needed; verified with the dependency-related test files (377-cyclic, 712/713- require_via, 121-prereq-module, 515-depends-on, 516-prereq-all, 517- always-load) plus the Tcl lint check, not the full suite. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
xdelaruelle
force-pushed
the
improve-tcl-coverage
branch
from
August 14, 2026 16:11
fc83cdf to
3192c02
Compare
The autosymbol block in getModules only defines an auto "latest" symbol for a directory element when no entry already exists for that name, or an existing one is in error -- in which case it still leaves the existing entry alone and only attempts resolution again. No existing fixture had a real "latest" symbolic version that was itself also forbidden, so the branch handling an existing (error) entry, as opposed to the plain not-found case, was never taken. Add autosym10: a module whose real "2" version is also named "latest" through an explicit module-version, with that exact symbolic name targeted by both module-forbid and a hard module-hide. Querying or loading it by the real name still resolves normally, but the "latest" name itself stays access-denied rather than falling back to an autosymbol pointing at the same module, since found_list already carries an (error) entry for it by the time the autosymbol phase runs. Verified with 'script/mt 70/274' (229 passes), plus the tests that enumerate this fixture's modulepath wholesale (avail/spider long listings, extra-spec/json/output tests) to confirm the new fixture does not change their expected output, not the full suite. Assisted-by: Claude:claude-sonnet-5 Signed-off-by: Xavier Delaruelle <xavier.delaruelle@cea.fr>
xdelaruelle
force-pushed
the
improve-tcl-coverage
branch
from
August 14, 2026 18:13
3192c02 to
9683519
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Codecov (https://app.codecov.io/gh/envmodules/modules/tree/main/tcl) flagged 26 uncovered lines across
tcl/. Working through them:siteconfig.tcl-1global-state injection, matching the pattern the suite already uses for config-option-shape and on-the-fly-procedure-redefinition coverage, for branches no real caller can currently reach).modfind.tcl.in: anelseifingetModules' version-symbol collection that became unreachable the moment it was added in 2020 (a sibling commit on the same day had already made theifabove it absorb the case unconditionally).report.tcl.in: an existence guard in the module-listing proc that's been dead since 2019 (the variable it checks is unconditionally initialized earlier in the same proc).modfind.tcl.in: an existence guard inunsetModuleDependency's unmet-dependency cleanup — the two structures it checks are always written and cleared together, so the guard can never be false.ifblocks. Those don't mean "block never entered" — Nagelfar/Codecov flag them when the condition has never evaluated false in any run. That reading changed the fix for several of them (forcing a skip path, not an entry path); details are in the individual commit messages.modfind.tcl.in's dependency-cycle refresh (setModuleDependency) has a narrow theoretical path viarequire_viamodulepath-swapping combined with a cycle that wasn't reachable through any load/unload/prereq sequence tried, and wasn't proven dead either. Left as-is.No behavior changes except the 3 dead-code removals, each verified to be a no-op.