On 3/19/25 11:25 PM, Rustam ALLAKOV wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: not tested
> Implements feature: tested, failed
> Spec compliant: tested, failed
> Documentation: tested, failed
>
> Hello everyone,
> for v2 patch I suggest to refactor from
>
>> -sp = subprocess.Popen(args.test_command, env=env_dict, stdout=subprocess.PIPE)
>> +if args.testname in ['regress', 'isolation', 'ecpg'] and 'EXTRA_REGRESS_OPTS' in env_dict:
>> + test_command = args.test_command + shlex.split(env_dict['EXTRA_REGRESS_OPTS'])
>> +else:
>> + test_command = args.test_command
>> +
>> +sp = subprocess.Popen(test_command, env=env_dict, stdout=subprocess.PIPE)
>
> to
>
> -sp = subprocess.Popen(args.test_command, env=env_dict, stdout=subprocess.PIPE)
> +if args.testname in ['regress', 'isolation', 'ecpg']:
> + test_command = args.test_command[:]
> + if 'TEMP_CONFIG' in env_dict:
> + test_command.insert(1, '--temp-config=' + env_dict['TEMP_CONFIG'])
> + if 'EXTRA_REGRESS_OPTS' in env_dict:
> + test_command += shlex.split(env_dict['EXTRA_REGRESS_OPTS'])
> +else:
> + test_command = args.test_command
> +
> +sp = subprocess.Popen(test_command, env=env_dict, stdout=subprocess.PIPE)
Since commit 51da766494dcc84b6f8d793ecaa064363a9243c2 it is possible
that we no longer need to use .insert() to make sure the code works on
Windows but I need to think a bit more on it.
But added support for TEMP_CONFIG.
> I double checked whether shlex module was built in Python, and yes it is,
> so no need for additional requirement.txt input for pip to install.
Thanks!
> in addition to the above, might be worth to add some documentation like
>
> Environment Variables Supported:
> EXTRA_REGRESS_OPTS: Additional options to pass to regression, isolation, or ecpg tests.
> TEMP_CONFIG: Specify a temporary configuration file for testing purposes.
> Example Usage:
> # Use EXTRA_REGRESS_OPTS to load an extension
> EXTRA_REGRESS_OPTS="--load-extension=pgcrypto" meson test
> # Use TEMP_CONFIG to specify a temporary configuration file
> TEMP_CONFIG="path/to/test.conf" meson test
> # Use both EXTRA_REGRESS_OPTS and TEMP_CONFIG together
> TEMP_CONFIG="path/to/test.conf" EXTRA_REGRESS_OPTS="--load-extension=pgcrypto" meson test
Yeah, we probably should. But not sure where, maybe at
https://www.postgresql.org/docs/current/install-meson.html or did you
imagine somewhere else?
> Should we cover these new lines with test? Asking, because I see EXTRA_REGRESS_OPTS
> being tested for autotools in src/interfaces/ecpg/test/makefile
As far as I can see they are not tested there, but maybe we should test
them.
Attached version 2 of the patch.
Andreas
