Cleanup test framework string formatting

Authored by thomie.


Cleanup test framework string formatting

  • Use format strings instead of string concatenation.
  • Wrap config.compiler, config.hpc etc. in quotes in mk/, so we don't have to in .T scripts and driver/

Update hpc submodule (test cleanup)

Reviewers: austin

Differential Revision:

pgj raised a concern with this commit.Mar 14 2015, 6:59 PM
pgj added a subscriber: pgj.

I think this change breaks the testsuite: the invocation of hp2ps will not work properly with shells where the brace expansion is not understood. I found this problem when browsing the testsuite output of my FreeBSD builders (such as [1]) and observed that basically every profasm way has failed due to this.

When I looked further into this problem, it turned out that the root of the problem is that hp2ps is invoked as {hp2ps} -- which might not be a problem for shells like Bash (available on almost every GNU/Linux and Mac OS X systems by default) but it certainly is for the (dumb) Bourne Shell which is the default on FreeBSD. And when I studied this, I had to come to the conclusion that this may not be the intended result at all -- as { and } are used as formatters in the Python code, but for the invocation of hp2ps, there is no variable named to insert into the format string.

Hence, in diff format, I would recommend a change something as follows (I have tested it, works for me):

--- a/testsuite/driver/
+++ b/testsuite/driver/
@@ -1573,7 +1573,7 @@ def write_file(file, str):
 def check_hp_ok(name):
     # do not qualify for hp2ps because we should be in the right directory
-    hp2psCmd = "cd " + getTestOpts().testdir + " && {hp2ps} " + name
+    hp2psCmd = "cd " + getTestOpts().testdir + (' && {hp2ps} ').format(hp2ps=config.hp2ps) + name
     hp2psResult = runCmdExitCode(hp2psCmd)

I do not consider myself a Python guru, so any better formulation is welcome :-) But now I hope you can see my concern with this commit. I am also inclined to think that there may be other similar mistakes lurking in the commit, so perhaps it may make sense to do another round of review.


Thank you for raising this; your assessment is completely correct. I have a patch up for review in D734. I'm currently buildilg a profiling compiler (might take a while), to be sure everything works as intended.

Not the first bug in this commit... I should be more careful with making changes to the testsuite driver.

thomie accepted this commit.Mar 17 2015, 6:38 AM

Back to about 170 unexpected test failures, same level as before this commit.

pgj added a comment.Mar 17 2015, 6:52 AM

Yes, indeed, thank you for fixing this! I have not yet used Phabricator much, so please bear with me: shall I do something with this, e.g. accept this commit or such?

Can you try selecting 'Accept commit' from the dropdown menu at the bottom of the page? Maybe that'll remove it from the list of "Problem commits".

pgj accepted this commit.Mar 17 2015, 7:02 AM