[PATCH] D99728: [lit, test] Fix test cancellation feature detection

[Julian Lettner via Phabricator via llvm-commits]

yln accepted this revision.
yln added a comment.
This revision is now accepted and ready to land.

Your overall explanation of the issue and the solution presented here makes sense to me, but I don't have a good enough understanding to know if this is the best approach.

LGTM with some small nits!
Please wait for a second sign-off or a few days before landing, so that other folks have time to take a look.



================
Comment at: llvm/utils/lit/tests/check-tested-lit-timeout-ability:1
+#!/usr/bin/python3.6
+
----------------



================
Comment at: llvm/utils/lit/tests/check-tested-lit-timeout-ability:9
+if not supported:
+    sys.exit(errormsg)
+
----------------
TIL: we can pass things other than integers to `sys.exit()`!  :)


================
Comment at: llvm/utils/lit/tests/check-tested-lit-timeout-ability:10-11
+    sys.exit(errormsg)
+
+sys.exit()
----------------
Not needed, right?


================
Comment at: llvm/utils/lit/tests/lit.cfg:83
+                      stderr=subprocess.PIPE, env=config.environment,
+                      universal_newlines=True)
+if proc.returncode == 0:
----------------
Please use `text=True` in favor of `universal_newlines=True`.

>From the docs:
> If encoding or errors are specified, or text is true, file objects for stdin, stdout and stderr are opened in text mode using the specified encoding and errors or the io.TextIOWrapper default. The universal_newlines argument is equivalent to text and is provided for backwards compatibility. By default, file objects are opened in binary mode.

We could also use `capture_output=True` instead of `stderr=subprocess.PIPE`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99728/new/

https://reviews.llvm.org/D99728