modules test not performing loads

[ben-bowers]

Describe the bug

In an attempt to refactor modulefiles into separate common modulefiles, ModuleTests no longer operate as expected.
Presumably this is because when run in test-mode the dependent modulefiles are not loaded. Since the top level module relies on a setenv from the sub-modulefile, the variable evaluates incorrectly and the test fails.

This may be a feature rather than a bug - but if there is some workaround, that would be helpful to know.

To Reproduce

Steps to reproduce the behavior:

$ module test fake_tool
-------------------------------------------------------------------
Module Specific Test for /home/bbowers/modulefiles/fake_tool:

/fake/bin
Test result: FAIL

For modules v4.3 to v4.7, the output is:

_UNDEFINED_/fake/bin
Test result: FAIL

Location and content of any modulerc or modulefile involved:

$ cat fake_base_path 
#%Module1.0 # -*- tcl -*-
setenv BASE_PATH /home/bbowers

$ cat fake_tool
#%Module1.0 # -*- tcl -*-
module load fake_base_path
set toolpath [getenv BASE_PATH]/fake/bin
prepend-path PATH $toolpath
proc ModulesTest {} {
    global toolpath
    puts stderr $toolpath
    return [file exist $toolpath]
}

Expected behavior

It would be convenient if sub-modulefiles are loaded as part of the module test so that it is possible to refactor common parts of the modulefiles. If that is considered breaking behavior, then maybe a mode that enables loads/prereq during the test?

Error and debugging information

$ module --debug <command1> <arguments>
$ module --debug <command2> <arguments>
...

Modules version and configuration

Tested across several versions:

$ module --version
Modules Release 4.3.1 (2019-09-21)
Modules Release 4.7.1 (2021-04-06)
Modules Release 5.0.0 (2021-09-12)

Additional context

[adrien-cotte]

Hi,

Could you explain why module load fake_base_path instead of prereq fake_base_path, please?

Best,
Adrien

[ben-bowers]

@adrien-cotte It is my understanding that there is no real difference between module load and prereq if MODULES_AUTO_HANDLING=1.

However, if that setting is not enabled then prereq requires that the sub-module load happened before loading the top level. Perhaps this is incorrect?

In any event, I tested this case with both prereq and module load with modules 4.3.1 and MODULES_AUTO_HANDLING=1 and the result was the same.

[adrien-cotte]

Ok, sorry as my config always been auto_handling on, I forget this is not the default behavior :)

Btw, I never test unloaded modules, is there any reason you cannot load your module before calling module test fake_tool?

With your example:

$ module test fake_tool
-------------------------------------------------------------------
Module Specific Test for /home/bbowers/modulefiles/fake_tool:

/fake/bin
Test result: FAIL
-------------------------------------------------------------------

$ module load fake_tool
$ module test fake_tool
-------------------------------------------------------------------
Module Specific Test for /home/bbowers/modulefiles/fake_tool:

/home/bbowers/fake/bin
Test result: PASS
-------------------------------------------------------------------

I don't really know why we have to load modules before testing, maybe we should ask of Module mailing list?

Does this workaround work for you?

[ben-bowers]

In many (most?) cases it's probably not necessary to load the modulefile. Having to do so seems cumbersome to me, as you'd (perhaps) have to be careful to load, test and unload.

Would the maintainers be interested in a PR to fix this issue? If so, is it necessary to be backward compatible with the existing behavior?

[xdelaruelle]

The g_modfilePerModeAliases table in initModfileModeAliases defines how the modulefile commands are implemented for each evaluation mode:

modules/tcl/mfinterp.tcl.in

Lines 90 to 130 in 3bc7b99

	set ::g_modfileEvalModes {load unload display help test whatis refresh}
	array set g_modfilePerModeAliases {
	add-property {nop nop nop nop nop nop nop }
	always-load {always-load nop reportCmd nop nop nop nop }
	append-path {append-path append-path-un append-path append-path append-path edit-path-wh nop }
	chdir {chdir nop reportCmd nop nop nop nop }
	complete {complete complete-un reportCmd nop nop nop complete }
	conflict {conflict nop reportCmd nop nop nop nop }
	depends-on {prereq-all nop reportCmd nop nop nop nop }
	extensions {nop nop nop nop nop nop nop }
	family {family family-un reportCmd nop nop nop nop }
	module {module module reportCmd nop nop nop nop }
	module-alias {module-alias module-alias module-alias module-alias module-alias module-alias nop }
	module-log {nimp nimp reportCmd nop nop nop nop }
	module-trace {nimp nimp reportCmd nop nop nop nop }
	module-user {nimp nimp reportCmd nop nop nop nop }
	module-verbosity {nimp nimp reportCmd nop nop nop nop }
	module-version {module-version module-version module-version module-version module-version module-version nop }
	module-virtual {module-virtual module-virtual module-virtual module-virtual module-virtual module-virtual nop }
	module-forbid {module-forbid module-forbid module-forbid module-forbid module-forbid module-forbid nop }
	module-hide {module-hide module-hide module-hide module-hide module-hide module-hide nop }
	module-tag {module-tag module-tag module-tag module-tag module-tag module-tag nop }
	module-whatis {nop nop reportCmd nop nop module-whatis nop }
	prepend-path {prepend-path prepend-path-un prepend-path prepend-path prepend-path edit-path-wh nop }
	prereq-all {prereq-all nop reportCmd nop nop nop nop }
	prereq-any {prereq nop reportCmd nop nop nop nop }
	prereq {prereq nop reportCmd nop nop nop nop }
	remove-path {remove-path remove-path-un remove-path remove-path remove-path edit-path-wh nop }
	remove-property {nop nop nop nop nop nop nop }
	require-fullname {require-fullname nop reportCmd nop nop nop nop }
	set-alias {set-alias set-alias-un reportCmd nop nop nop set-alias }
	set-function {set-function set-function-un reportCmd nop nop nop set-function}
	setenv {setenv setenv-un setenv setenv setenv setenv-wh nop }
	source-sh {source-sh source-sh-un source-sh-di nop nop nop source-sh }
	system {system system reportCmd nop nop nop nop }
	uncomplete {uncomplete nop reportCmd nop nop nop nop }
	unset-alias {unset-alias nop reportCmd nop nop nop nop }
	unset-function {unset-function nop reportCmd nop nop nop nop }
	unsetenv {unsetenv unsetenv-un unsetenv unsetenv unsetenv unsetenv-wh nop }
	variant {variant variant variant variant variant variant-wh variant }
	x-resource {x-resource x-resource reportCmd nop nop nop nop }

module, prereq, prereq-all, prereq-any, depends-on and conflict modulefile commands are currently implemented as a no-operation on test evaluation mode. test mode is made as close as help evaluation mode currently.

PR would be interesting to add the proposed new behavior. I still need to think if this should be the new default behavior or if an option is needed to enable such change.

[adrien-cotte]

So, actually module test evaluation is close to module help?
As I'm used to module load before module test, my opinion is that this PR makes sense.
Maybe an option to select what should be not evaluated during tests. Default value as current behavior, until Module next big release?

[xdelaruelle]

PR would be interesting to add the proposed new behavior. I still need to think if this should be the new default behavior or if an option is needed to enable such change.

Maybe an option to select what should be not evaluated during tests. Default value as current behavior, until Module next big release?

After some thought, I suggest the addition of a new configuration option, named test_mode_mimic (load or help as accepted values). Option should be set to help by default. When set to load, test evaluation mode should mimic a load evaluation mode, but not change user environment in the end of this processing.

@ben-bowers You can follow the Add new configuration option guide to craft this pull request.

[ben-bowers]

I believe I have something that barely works. The change to the Tcl is relatively small, but the files that needed to be updated were quite large. Thanks for the excellent Add New configuration option guide!

One place I find that it is not working as expected is when I switch to use the load entry from the g_modfilePerModeAliases, the prereq command is working as in load mode, but the module load command is not.

For instance, and the test_mode_mimic==load and the top modulefile is defined as:

#%Module1.0 # -*- tcl -*-
module load fake_base_path ## <--- doesnt evaluate this file
set toolpath [getenv BASE_PATH]/fake/bin

the fake_base_path is not evaluated.

However when test_mode_mimic==load and the top modulefile is defined as:

#%Module1.0 # -*- tcl -*-
prereq fake_base_path ## <--- works!!
set toolpath [getenv BASE_PATH]/fake/bin

then the evaluation of the file happens.

I had previously thought that these would have worked the same, but they call different tcl commands under the covers, so apparently it is not straightforward.

The other side effects I need to figure out are:

1. modules prints Loading fake_base_path, which is probably not something desired during test mode
2. modules is actually loading the fake_base_path lower level file during test mode and setting the env-variables in the shell, which is not desirable

[xdelaruelle]

For your first point, the module procedure (in main.tcl.in) adapts it behavior depending on the current evaluation mode found:

modules/tcl/main.tcl.in

Lines 364 to 365 in aa33eda

	# get mode, set to load if called from top level
	set mode [expr {$topcall ? {load} : [currentState mode]}]

For the change your are crafting, I suggest you adapt the mode variable just below these code lines with:

# set evaluation mode to the mode to mimic on test mode 
if {$mode eq {test} && [getConf test_mode_mimic] eq {load}} {
   set mode load
}

This way module should operate in test mode as if was called in load mode if test_mode_mimic is set to load.

[xdelaruelle]

For the second side effect mentioned, a clearSettings procedure could be added in envmngt.tcl.in file (above renderSettings procedure).

This new procedure could array unset the following global arrays:

• g_stateEnvVars
• g_stateAliases
• g_stateFunctions
• g_stateCompletes
• g_newXResources
• g_delXResources

Then the end of the module procedure should be updated (in main.tcl.in):

modules/tcl/main.tcl.in

Lines 754 to 756 in aa33eda

	# if called from top level render settings if any
	if {$topcall} {
	renderSettings

It should be updated to something like:

if {$topcall} {
   # clear env changes if test mode mimics load mode
   if {[currentState mode] eq {test} && [getConf test_mode_mimic] eq\
      {load}} {
      clearSettings
   }                                                                                 
   renderSettings                                                                                     
}

[xdelaruelle]

For the Loading ... message I would need the PR code to see how the messages could be inhibited in this situation.