Re: PG_TEST_EXTRA and meson

Thanks Nazir,

> > -# Test suites that are not safe by default but can be run if selected
> > -# by the user via the whitespace-separated list in variable PG_TEST_EXTRA.
> > -# Export PG_TEST_EXTRA so it can be checked in individual tap tests.
> > -test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))
> >
> > A naive question. What happens if we add PG_TEST_EXTRA in meson.build
> > itself rather than passing it via testwrap? E.g. like
> > if "PG_TEST_EXTRA" not in os.environ
> > test_env.set('PG_TEST_EXTRA', get_option('PG_TEST_EXTRA'))
>
> Then this configure time option will be passed to the test environment
> and there is no way to change it without reconfiguring if we don't
> touch the testwrap file.
>
> > I am worried that we might have to add an extra argument to testwrap
> > for every environment variable that influences the tests. Avoiding it
> > would be better.
>
> If we want to have both configure time and run time variables, I
> believe that this is the only way for now.

Here's what I understand, please correct me: The code in meson.build
is only called at the time of setup; not during meson test. Hence we
can not check the existence of a runtime environment variable in that
file. The things in test_env override those set at run time. So we
save it as an argument to --pg_test_extra and then use it if
PG_TEST_EXTRA is not set at run time. I tried to find if there's some
other place to store "environment variables that can be overriden at
runtime" but I can not find it. So it looks like this is the best we
can do for now.

If it comes to a point where there are more such environment variables
that need to be passed, probably we will pass a key-value string of
those to testwrap. For now, for a single variable, this looks ok.

>
> > +# If PG_TEST_EXTRA is not set in the environment, then look for its Meson
> > +# configure option.
> > +if "PG_TEST_EXTRA" not in os.environ and args.pg_test_extra:
> > + env_dict["PG_TEST_EXTRA"] = args.pg_test_extra
> > +
> >
> > If somebody looks at just these lines, it's not clear how
> > PG_TEST_EXTRA is passed to the test environment if it's available in
> > os.environ. So one has to understand that env_dict is the test
> > environment. If that's so, the code and comment rewritten as below
> > makes more sense to me. What do you think?
> >
> > # If PG_TEST_EXTRA is not already part of the test environment, check if it's
> > # passed via program argument --pg_test_extra. Thus we also override
> > # configuration time value by run time value of PG_TEST_EXTRA.
> > if "PG_TEST_EXTRA" not in env_dict and args.pg_test_extra:
> > env_dict["PG_TEST_EXTRA"] = args.pg_test_extra
>
> I think your suggestion is better, done.

I didn't see the expected change. I was talking about something like attached.

Also
1. I have also made changes to the comment,
2. renamed the argument --pg_test_extra to --pg-test-extra using
convention similar to other arguments.
3. few other cosmetic changes.

Please review and incorporate those in the respective patches and
tests. Sorry for a single diff.

Once this is done, I think we can mark this CF entry as RFC.

--
Best Wishes,
Ashutosh Bapat