Page MenuHomePhabricator

[lit, test] Fix test cancellation feature detection
ClosedPublic

Authored by thopre on Apr 1 2021, 7:14 AM.

Details

Summary

A lit feature guards tests for the lit timeout functionality because on
most system it depends on the availability of the psutil Python module.
However, that feature is defined based on the ability of the testing lit
to cancel test, which does not necessarily apply to the ability of the
tested lit.

In particular, RUN commands have a cleared PYTHONPATH and user site
packages are disabled. In the case where psutil is found by the testing
lit from one of those two source of python path, the tested lit would
not be able to find it, causing timeout tests to fail.

This commit fixes the issue by testing the ability to cancel tests in
the RUN command environment.

Diff Detail

Event Timeline

thopre created this revision.Apr 1 2021, 7:14 AM
thopre requested review of this revision.Apr 1 2021, 7:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 1 2021, 7:14 AM
yln accepted this revision.Apr 1 2021, 7:10 PM

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.

llvm/utils/lit/tests/check-tested-lit-timeout-ability
2
10

TIL: we can pass things other than integers to sys.exit()! :)

11–12

Not needed, right?

llvm/utils/lit/tests/lit.cfg
83

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.

This revision is now accepted and ready to land.Apr 1 2021, 7:10 PM
thopre added inline comments.Apr 2 2021, 2:26 AM
llvm/utils/lit/tests/lit.cfg
83

Both of these require Python 3.7 and thus fail on Ubuntu 18.04 and RHEL

yln added inline comments.Apr 2 2021, 1:20 PM
llvm/utils/lit/tests/lit.cfg
83

You are right! Please ignore my comment.

thopre updated this revision to Diff 336048.Apr 8 2021, 3:27 AM

Simplify logic

thopre updated this revision to Diff 336053.Apr 8 2021, 3:54 AM

Do not rely on testing script being executable

thopre updated this revision to Diff 336054.Apr 8 2021, 3:56 AM

Do rely on script being executable but not on Python 3.6 being in PATH, only Python3

thopre marked 3 inline comments as done.Apr 8 2021, 3:58 AM
thopre updated this revision to Diff 336218.Apr 8 2021, 2:01 PM

Call script with python3 to not rely on it being executable

thopre updated this revision to Diff 337260.Apr 13 2021, 2:34 PM

Add print statement

thopre updated this revision to Diff 337369.Apr 14 2021, 1:39 AM
  • Use exception to see file
thopre updated this revision to Diff 337533.Wed, Apr 14, 1:12 PM

Use slash as path separator

thopre updated this revision to Diff 337554.Wed, Apr 14, 2:06 PM

Fix joining

thopre updated this revision to Diff 337880.Thu, Apr 15, 1:09 PM

Print not found file's filename

jhenderson added inline comments.Mon, Apr 19, 6:52 AM
llvm/utils/lit/tests/lit.cfg
83

Does sys.executable instead of "python3" work for your problem (I am not familiar enough with the issue to be sure)? It feels nicer to me, although I can't say any particular reason why.

thopre added inline comments.Mon, Apr 19, 8:01 AM
llvm/utils/lit/tests/lit.cfg
83

I'm not sure what's the problem at the moment. There's a file not found exception on Windows but I have no idea why. I could give it a try and see.

thopre updated this revision to Diff 338528.Mon, Apr 19, 8:21 AM

use sys.executable to find Python executable

thopre updated this revision to Diff 338767.Tue, Apr 20, 1:39 AM

Remove debuging code now that test pass on Windows as well.

thopre marked an inline comment as done.Tue, Apr 20, 1:40 AM
thopre added inline comments.
llvm/utils/lit/tests/lit.cfg
83

Many thanks, that was the missing bit. I focused on the script but it was Python3 it could not find.

This revision was automatically updated to reflect the committed changes.