Re: Add support for EXTRA_REGRESS_OPTS for meson

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