Rework testing under pytest by t-bltg · Pull Request #71 · vstinner/python-ptrace

t-bltg · 2021-02-05T13:13:57Z

~~- Rework documentation test so that $ pytest catches 27f1472.~~

Added strace.py tests for directory tests/crash (must check if they do not break tox tests added by Added automatic tests on push and pull requests #70).

$ pytest
============================= test session starts ==============================
platform linux -- Python 3.9.1, pytest-6.2.2, py-1.10.0, pluggy-0.13.1
rootdir: $SRC_DIR
collecting ... 
collected 19 items                                                             

tests/test_crash.py .                                                    [  5%]
tests/test_doc.py .                                                      [ 10%]
tests/test_gdb.py .......                                                [ 47%]
tests/test_strace.py ..........                                          [100%]

============================== 19 passed in 8.44s ==============================

spoutn1k · 2021-02-05T16:51:29Z

The failure comes from the deletion of test_doc.py that is required in tox.ini. Please edit the file to reflect the changes.

Also, on my machine the module is globally installed, and only running pytest like in your example tests the installation rather than the directory. Please be careful of this. tox is still the only documented testing method, consider adding an entry for pytest.

spoutn1k · 2021-02-05T18:15:08Z

Why rename test_doc.py to check_doc.py ? This change affects 3 files with no other modifications.

t-bltg · 2021-02-05T18:16:32Z

Thanks, I'm not familiar with tox, more with pytest (I initially rewrote test_doc.py to unittest because the file was picked by pytest's discovery). Let's rename it to check_doc.py ?.

spoutn1k · 2021-02-05T18:20:05Z

You may not be but is pytest really necessary ? Victor made the project with tox and adding pytest seems redundant. Does pytest have coverage on anything tox does not ?

Edit - my bad, I made the assumption that tox and pytest were the same general kind of programs, while tox can actually manage pytest runs. Thus we should prioritize running with test tox, and adding pytest as a test driver can be useful. tox still has more features as a test manager - namely virtual environments and embarked parallelism.

t-bltg · 2021-02-05T18:35:51Z

You may not be but is pytest really necessary ?

I have issues running tox locally, whereas the pytest commands runs fine. Tox sets up a virtualenv and install dependencies, before running the test in a virtualenv installation, I guess pytest is more oriented towards development in the current directory.

[...]
----------------------------------------------------------------------
Ran 18 tests in 7.471s

OK
pep8 create: $SRC_DIR/.tox/pep8
pep8 installdeps: flake8
ERROR: invocation failed (exit code 1), logfile: $SRC_DIR/.tox/pep8/log/pep8-1.log
================================== log start ===================================
ERROR: Could not find a version that satisfies the requirement flake8
ERROR: No matching distribution found for flake8

=================================== log end ====================================
ERROR: could not install deps [flake8]; v = InvocationError('$SRC_DIR/.tox/pep8/bin/python -m pip install flake8', 1)
___________________________________ summary ____________________________________
  py3: commands succeeded

spoutn1k · 2021-02-05T18:37:28Z

I have issues running tox locally, whereas the pytest commands runs fine.

~~Well flake8 is not installed and fails to install, try running python -m pip install flake8 to see the exact error.~~

Interesting, the sub-installation failed, but this is the formatting check. Does tox have access to the internet on this machine ?

t-bltg · 2021-02-05T18:44:06Z

~~Well flake8 is not installed and fails to install, try running python -m pip install flake8 to see the exact error.~~

Interesting, the sub-installation failed, but this is the formatting check. Does tox have access to the internet on this machine ?

It's a virtualenv deployement issue with pip, yes flake8 is installed. Am digging into this, but the logs aren't very helpful ...

spoutn1k · 2021-02-05T18:47:52Z

Worst case scenario, skip the format check using tox -e py3. Let the github workflow handle the format check.

t-bltg · 2021-02-12T17:57:55Z

Hi @spoutn1k , @vstinner, do you have any objection to merging this PR ?

vstinner · 2021-02-15T07:54:10Z

tests/crash/stack_overflow.c

 {
-    char buffer[4096];
-    buffer[0] = 0;
+    char buffer[4096] = {0};


To test the Python faulthandler module of the Python stdlib, I wrote:

volatile unsigned char buffer[4096]; buffer[0] = 1; buffer[4095] = 0;

https://github.com/python/cpython/blob/fcbe0cb04d35189401c0c880ebfb4311e952d776/Modules/faulthandler.c#L1132-L1147

volative prevents compilation optimizations:

/* Allocate (at least) 4096 bytes on the stack at each call. bpo-23654, bpo-38965: use volatile keyword to prevent tail call optimization. */

OK, I have changed this.

vstinner · 2021-02-15T07:54:53Z

tests/test_strace.py


 AARCH64 = (getattr(os.uname(), 'machine', None) == 'aarch64')

+UNSUPPORTED = platform.system() not in ('Linux',)


python-ptrace supports FreeBSD, no? Why only running strace.py tests on Linux?

Yes, but the github actions do not support FreeBSD, and I have no way of testing this. I took a conservative approach, avoiding to break tests untested systems.

If the test is skipped and someone runs the tests manually, it's too easy to miss that the whole test is skipped. I prefer to continue to run the test, and only skip a specific test if there is no way to fix it.

But isn't it what I do here ? I only skip a single test: @unittest.skipIf(UNTESTED, 'Untested system/OS') ...

vstinner · 2021-02-15T07:56:01Z

tests/crash/run.sh

+    python ../../strace.py -e execve $1; ec=$?
+    if [ $ec -gt 0 ]; then
+      exit $(($ec - 128 - $2))
+    fi


Why not writing the tests in pure Python in test_strace.py?

I thought that having the possibility to run these tests outside the python testing framework was useful (either for dev or for comprehension). If not, tell me, I will rewrite them in pure Python.

I don't feel the need, it's up to you. I'm trying to not be a control freak :-)

vstinner · 2021-02-15T16:48:56Z

tests/test_strace.py

+    @unittest.skipIf(UNTESTED, 'Untested system/OS')
+    def test_crash(self):
+        dn = os.path.join(os.path.dirname(__file__), 'crash')
+        shell = shutil.which('bash')


Well, that's one reason to not rely on a shell, to be able to run tests even if bash is not available ;-)

Ok, but this test relies on having a proper toolchain (at least gcc, make, ...) so bash is a weak dependency. Or maybe if the directory tests/crash is present but not used by any test (dead code) it should be removed ?

vstinner · 2021-02-15T16:49:25Z

tests/crash/run.sh

+    python ../../strace.py -e execve $1; ec=$?
+    if [ $ec -gt 0 ]; then
+      exit $(($ec - 128 - $2))
+    fi


I don't feel the need, it's up to you. I'm trying to not be a control freak :-)

spoutn1k · 2021-02-15T16:52:38Z

Could you remove the github workflow about pytest ? You squashed my commit so I cannot remove it cleanly. It is fine if you prefer to use pytest, but tox is still the preferred and feature-complete way of checking the code.

vstinner · 2021-02-15T17:26:54Z

You can use python3 -c 'import signal; signum=signal.SIGABRT; signal.signal(signum, signal.SIG_DFL); signal.raise_signal(signum)' (or write a script for that) for example to get a program killed by a signal in a reliable way. So you don't need make/gcc ;-) For faulthandler, I expose private functions in a C extensions for that, like faulthandler._sigsegv().

t-bltg · 2021-02-15T17:31:34Z

Could you remove the github workflow about pytest ? You squashed my commit so I cannot remove it cleanly. It is fine if you prefer to use pytest, but tox is still the preferred and feature-complete way of checking the code.

Sorry about squashing and cleaning up old commits. It's done.

t-bltg · 2021-02-15T17:59:38Z

You can use python3 -c 'import signal; signum=signal.SIGABRT; signal.signal(signum, signal.SIG_DFL); signal.raise_signal(signum)' (or write a script for that) for example to get a program killed by a signal in a reliable way. So you don't need make/gcc ;-) For faulthandler, I expose private functions in a C extensions for that, like faulthandler._sigsegv().

Ok, let's forget about running a shell script in the python driven tests.

t-bltg force-pushed the testing branch 4 times, most recently from b9477c7 to 6c77f84 Compare February 5, 2021 18:12

vstinner reviewed Feb 15, 2021

View reviewed changes

t-bltg force-pushed the testing branch 2 times, most recently from 6d5180a to 4c6ab71 Compare February 15, 2021 15:44

vstinner reviewed Feb 15, 2021

View reviewed changes

t-bltg force-pushed the testing branch from 4c6ab71 to 327b84b Compare February 15, 2021 17:31

t-bltg force-pushed the testing branch from 327b84b to a1a5786 Compare February 15, 2021 17:58

rework testing under tox

a7762f5

t-bltg force-pushed the testing branch from a1a5786 to a7762f5 Compare February 15, 2021 18:00

Base automatically changed from master to main March 17, 2021 20:14


		AARCH64 = (getattr(os.uname(), 'machine', None) == 'aarch64')

		UNSUPPORTED = platform.system() not in ('Linux',)

Conversation

t-bltg commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spoutn1k commented Feb 5, 2021

Uh oh!

spoutn1k commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bltg commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spoutn1k commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bltg commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spoutn1k commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t-bltg commented Feb 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spoutn1k commented Feb 5, 2021

Uh oh!

t-bltg commented Feb 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-bltg Feb 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spoutn1k commented Feb 15, 2021

Uh oh!

vstinner commented Feb 15, 2021

Uh oh!

t-bltg commented Feb 15, 2021

Uh oh!

t-bltg commented Feb 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

t-bltg commented Feb 5, 2021 •

edited

Loading

spoutn1k commented Feb 5, 2021 •

edited

Loading

t-bltg commented Feb 5, 2021 •

edited

Loading

spoutn1k commented Feb 5, 2021 •

edited

Loading

t-bltg commented Feb 5, 2021 •

edited

Loading

spoutn1k commented Feb 5, 2021 •

edited

Loading

t-bltg commented Feb 5, 2021 •

edited

Loading

t-bltg Feb 15, 2021 •

edited

Loading