-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework testing under pytest #71
base: main
Are you sure you want to change the base?
Conversation
The failure comes from the deletion of Also, on my machine the module is globally installed, and only running |
b9477c7
to
6c77f84
Compare
Why rename |
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 |
You may not be but is Edit - my bad, I made the assumption that |
I have issues running
|
Interesting, the sub-installation failed, but this is the formatting check. Does tox have access to the internet on this machine ? |
It's a |
Worst case scenario, skip the format check using |
tests/crash/stack_overflow.c
Outdated
@@ -1,7 +1,6 @@ | |||
char toto() | |||
{ | |||
char buffer[4096]; | |||
buffer[0] = 0; | |||
char buffer[4096] = {0}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To test the Python faulthandler module of the Python stdlib, I wrote:
volatile unsigned char buffer[4096];
buffer[0] = 1;
buffer[4095] = 0;
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. */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I have changed this.
tests/test_strace.py
Outdated
|
||
STRACE = os.path.normpath( | ||
os.path.join(os.path.dirname(__file__), '..', 'strace.py')) | ||
|
||
AARCH64 = (getattr(os.uname(), 'machine', None) == 'aarch64') | ||
|
||
UNSUPPORTED = platform.system() not in ('Linux',) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
python-ptrace supports FreeBSD, no? Why only running strace.py tests on Linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't it what I do here ? I only skip a single test: @unittest.skipIf(UNTESTED, 'Untested system/OS')
...
python ../../strace.py -e execve $1; ec=$? | ||
if [ $ec -gt 0 ]; then | ||
exit $(($ec - 128 - $2)) | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not writing the tests in pure Python in test_strace.py?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel the need, it's up to you. I'm trying to not be a control freak :-)
6d5180a
to
4c6ab71
Compare
tests/test_strace.py
Outdated
@unittest.skipIf(UNTESTED, 'Untested system/OS') | ||
def test_crash(self): | ||
dn = os.path.join(os.path.dirname(__file__), 'crash') | ||
shell = shutil.which('bash') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's one reason to not rely on a shell, to be able to run tests even if bash is not available ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
python ../../strace.py -e execve $1; ec=$? | ||
if [ $ec -gt 0 ]; then | ||
exit $(($ec - 128 - $2)) | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel the need, it's up to you. I'm trying to not be a control freak :-)
Could you remove the github workflow about |
You can use |
Sorry about squashing and cleaning up old commits. It's done. |
Ok, let's forget about running a shell script in the python driven tests. |
- Rework documentation test so that$ pytest
catches 27f1472.strace.py
tests for directorytests/crash
(must check if they do not break tox tests added by Added automatic tests on push and pull requests #70).