Skip to content

Conversation

@ildus
Copy link

@ildus ildus commented Apr 9, 2019

No description provided.

Copy link
Owner

@funbringer funbringer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for a nice feature! Can't wait to merge this when you're finished with dealing with those nitpicks.

isolation_args = [
os.path.join(instance.work_dir,
"src/test/isolation/pg_isolation_regress"),
"--temp-instance=%s" % tmpdir,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshedding: I personally prefer format to %, since this is a recommended way of string formatting in 3.0+. Could you rewrite those pieces of code, please?


self._regress_opts = None

def get_temp_config(self):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is a perfect candidate for being a property.


def make(self, targets=None, options=None):
self.specs = None
specs_dir = os.path.join(work_dir, 'specs')
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably shouldn't hardwire specs dir name. Maybe we could add a default-able flag to the pgxs command?

"src/test/isolation/pg_isolation_regress"),
"--temp-instance=%s" % tmpdir,
"--inputdir=.",
"--outputdir=output_iso",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needless contraction -- output_isolation?

if target in ('check', 'installcheck') and self.specs:
print(Style.green('\n$ make isolationcheck'))

tmpdir = os.path.join(self.work_dir, "tmp_check_iso")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use mkdtemp to make sure that dir name is unique? Otherwise it could result in name clash when used in parallel builds.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants