Skip to content
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

tests: add --dev null test suite #541

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

mattock
Copy link
Member

@mattock mattock commented Apr 23, 2024

This PR adds a --dev null test suite. The implementation has been described in doc/t_server_null.rst. In addition the scripts themselves include plenty of documentation on what is happening.

This particular incarnation of --dev null has been stress-tested for ~2000 rounds with zero failure rate. Currently tested platforms are Fedora Linux 38 and FreeBSD 14. Any changes to the test suite, in particular its logic, will need to be exposed to a fairly comprehensive tests to avoid subtle issues; for example, in one case a small bug caused ~1% test failure rate - a rate which is impossible to notice without hundreds or so rounds of stress-testing.

@mattock mattock force-pushed the dev_null branch 2 times, most recently from a3e49e3 to 9554d06 Compare April 23, 2024 07:47
@mattock
Copy link
Member Author

mattock commented Apr 23, 2024

Made changes based on openvpn-devel IRC discussions:

  • Used the same privilege escalation mechanism as in t_client.sh
  • Kill server instances using the info in their pid files instead of netcat
  • Fix some documentation/comment issues

@flichtenheld
Copy link
Member

Misses changes to github config to actually run the new tests.

@flichtenheld flichtenheld self-requested a review April 23, 2024 12:24
Copy link
Member

@flichtenheld flichtenheld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely not tested with out-of-tree build ;)

tests/Makefile.am Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_server.sh Outdated Show resolved Hide resolved
tests/t_server_null_client.sh Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_server.sh Show resolved Hide resolved
tests/t_server_null_server.sh Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Outdated Show resolved Hide resolved
tests/t_server_null_default.rc Show resolved Hide resolved
@@ -0,0 +1,65 @@
# Notes regarding --dev null server and client configurations:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Notes regarding --dev null server and client configurations:
# -*- shell-script -*-
# Notes regarding --dev null server and client configurations:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to have similar declarations/hints elsewhere, namely -*- Autoconf -*- in .m4 files and -*- coding: utf-8 -*- in Python files. I see how this might be useful in some context as this is a shell script without an explicit #!/bin/sh at the top. However, I think we should also add this to t_client.rc-sample to be consistent with these markers. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't hurt

@mattock
Copy link
Member Author

mattock commented May 7, 2024

@flichtenheld I applied your suggestions and ran in-tree and out-of-tree builds and did some stress testing just in case. Everything seemed to work fine. Perhaps we're close to the point where these changes could be wrapped into a patch (series) and sent to openvpn-devel?

mattock and others added 16 commits May 14, 2024 20:32
Signed-off-by: Samuli Seppänen <[email protected]>
This removes the dependency to netcat and is more in line with the rest of the
scripts.

Signed-off-by: Samuli Seppänen <[email protected]>
The code is borrowed from t_client.sh

Signed-off-by: Samuli Seppänen <[email protected]>
The code is mostly copy-and-paste from t_client.sh

Signed-off-by: Samuli Seppänen <[email protected]>
Both t_server_null.sh and t_client.sh created network interfaces. This worked
fine when the tests ran in serial, but broke t_client.sh when make ran tests in
parallel mode. This commit fixes this by ordering the two test suites so that
they never run at the same time, and by ensuring that server instances launched
by t_server_null.sh are completely shut down before t_client.sh starts.

Signed-off-by: Samuli Seppänen <[email protected]>
@mattock
Copy link
Member Author

mattock commented May 14, 2024

@flichtenheld I believe the parallel make issue is now gone, see latest commit.

@mattock
Copy link
Member Author

mattock commented May 16, 2024

@flichtenheld I believe the parallel make issue is now gone, see latest commit.

Note that t_client.sh seems to fail if the host "make check" runs on does not have IPv6. That caught me by surprise yesterday.

@mattock
Copy link
Member Author

mattock commented Jun 4, 2024

@flichtenheld I believe this PR is now ready. Care to check?

tests/Makefile.am Outdated Show resolved Hide resolved
Co-authored-by: Frank Lichtenheld <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants