Add CMake targets for integration tests and switch CI to use them #3776
Conversation
harrisonkaiser
force-pushed
the
cmake-integv2
branch
from
f4f7c9a to
ba368e6
Compare
harrisonkaiser
requested review from
goatgoose,
maddeleine,
dougch and
jmayclin
and removed request for
dougch
|
Perhaps this is a bit out of left field, but what are the advantages of cmake vs just directly invoking the python stuff and driving it from a shell script / python script? Just wondering whether cmake is the neatest/simplest solution for us. |
The goal is to provide a way for developers to run the integ tests (as a group and individually) independent of codebuild. Right now we have a bunch of stuff implicitly already setup by docker and explicitly setup by a script. As a result, it is very difficult to replicate CI locally or setup a fully working development environment. The thought behind using CMake is that it could do all the setup environment for the developer. I'd be happy with any solution that effectively achieves that goal. |
harrisonkaiser
force-pushed
the
cmake-integv2
branch
from
f082e10 to
0dca07e
Compare
harrisonkaiser
changed the title
harrisonkaiser
changed the title
| LD_LIBRARY_PATH="${PROJECT_SOURCE_DIR}/libcrypto-root/lib":"${PROJECT_SOURCE_DIR}/test-deps/openssl-1.1.1/lib":"${PROJECT_SOURCE_DIR}/test-deps/gnutls37/nettle/lib":$ENV{LD_LIBRARY_PATH} | ||
| PATH="${PROJECT_SOURCE_DIR}/bin":"${PROJECT_SOURCE_DIR}/test-deps/openssl-1.1.1/bin":"${PROJECT_SOURCE_DIR}/test-deps/gnutls37/bin":$ENV{PATH} | ||
| PYTHONNOUSERSITE=1 | ||
| S2N_INTEG_TEST=1 |
There was a problem hiding this comment.
Out of the scope of this PR, but I'll also throw out my unsolicited opinion that using special code paths for all integration tests scares me.
This is probably a bit paranoid, but it looks like setting this environment variable let's you call s2n_config_set_unsafe_for_testing. If we were to add a regression that resulted in a production code path calling s2n_config_set_unsafe I don't think we wouldn't catch that in unit or integ tests, because it's explicitly allowed for all of them.
As a future solution maybe we could only set S2N_INTEG_TEST=1 on the tests that actually need it? That could probably be driven from the python layer.
If others agree I can create an issue tracking this.
This is a step towards our goal of enabling CMake for all parts of the code base. The CMake script now adds integ_<name> target for each `test_<name>.py` in `tests/integrationv2/`. A developer can now run `cmake --build <path/to/build> --target test_name` to run an individual integration test. We don't remove the existing Makefile.
harrisonkaiser
force-pushed
the
cmake-integv2
branch
from
fc45616 to
f76c17e
Compare
harrisonkaiser
requested review from
dougch and
camshaft
and removed request for
camshaft
| cmake --build ./build --target clean #Saves on copy back rsync time |
There was a problem hiding this comment.
You're going to get a conflict with #3837
|
Changes post approval lgtm. |
Resolved issues:
Description of changes:
Move to using CMake to run integration tests in s2n_small batch. Currently this PR does a straight move from one to the other.
One option, not implemented here, is to run the two methods in parallel.
Call-outs:
CMake has a feature find_package which we use here to find the Python3 executable. This is helpful if you are running the executable in a development environment. However our VM's don't appear to be setup (i.e. CMAKE_PREFIX_PATH is not set) to make it easy for CMake to find the Python, we override this with s2n_codebuild.sh. Would love to hear Ideas about a better solution to this.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
Is this a refactor change? If so, how have you proved that the intended behavior hasn't changed?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.