public inbox for [email protected]  

help / color / mirror / Atom feed

From: Rustam ALLAKOV <[email protected]>
To: [email protected]
Cc: Andreas Karlsson <[email protected]>
Subject: Re: Add support for EXTRA_REGRESS_OPTS for meson
Date: Wed, 19 Mar 2025 22:25:46 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <CALDaNm1G3qSNgVA2543BmB+dimT7FLGYksY5iLBmcLe8Rj-1dg@mail.gmail.com>
References: <[email protected]>
	<CALDaNm1G3qSNgVA2543BmB+dimT7FLGYksY5iLBmcLe8Rj-1dg@mail.gmail.com>

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)

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.

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

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

Regards.

view thread (9+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected]
  Subject: Re: Add support for EXTRA_REGRESS_OPTS for meson
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox