create_key test: add missing tests #132
Conversation
In preparation for future work, add a few tests that are missing from `create_key`, specifically tests that verify the request.cnf, permissions.p7s, governance.p7s, and ecdsaparam. Also rework test module to use module fixtures and multiple smaller test cases instead of one large one. This makes it easier to add more, and makes test failures more meaningful. Signed-off-by: Kyle Fazzari <kyle@canonical.com>
kyrofa
force-pushed
the
feature/missing_create_key_tests
branch
from
c431612
to
2612e0e
Compare
Signed-off-by: Kyle Fazzari <kyle@canonical.com>
| from sros2.api import create_keystore | ||
|
|
||
|
|
||
| # This fixture will run once for the entire module (as opposed to once per test) | ||
| @pytest.fixture(scope='module') | ||
| def node_keys_dir(tmp_path_factory): |
There was a problem hiding this comment.
How does this function get the argument tmp_path_factory?
I am asking because the nightly CI jobs fail the test: http://build.ros2.org/view/Eci/job/Eci__nightly-fastrtps_ubuntu_bionic_amd64/19/testReport/sros2.test.sros2.commands.security.verbs/test_create_key/test_create_key/
The nightly job on ci.ros2.org seems to pass (https://ci.ros2.org/view/nightly/job/nightly_linux_debug/1254/testReport/sros2.test.sros2.commands.security.verbs/test_create_key/test_create_key/). Not sure what is different between those.
There was a problem hiding this comment.
Well that's an interesting failure. This is a built-in pytest fixture. Any chance the version of pytest differs between those two tests?
There was a problem hiding this comment.
I faced multiple issues about mismatching versions of pytest recently. My guess is that one of these jobs installs a recent pytest from pip and the other one an older version from apt
There was a problem hiding this comment.
That is certainly the case: one uses the Debian package from Ubuntu Bionic, the other uses the latest release (currently we pinned 4.x instead of using 5 - but that is a different reason). So the code should work with either of the versions.
There was a problem hiding this comment.
Good theory, the one from apt may need to also install python3-pytest-tempdir (total guess based on the name, but seems plausible).
There was a problem hiding this comment.
Good theory, the one from apt may need to also install python3-pytest-tempdir (total guess based on the name, but seems plausible).
👍 that would make sense. To do so it will need to be listed as a test dependency in sros2's package.xml and added to the rosdep database for rosdep to resolve it properly.
There was a problem hiding this comment.
Yeah, too bad they're split out like that 🤷♂️ . Let me confirm the theory and I'll take care of it if that happens to be the case. @dirk-thomas, is there a way to run the failing test on a pull request instead of the one that passes? Can we do something to bring those testbeds more closely inline with each other?
There was a problem hiding this comment.
is there a way to run the failing test on a pull request instead of the one that passes?
The master branch (which will become Eloquent) doesn't have PR jobs yet. So there is no automated way to do that. If you make a PR we can manually trigger a job for it (http://build.ros2.org/view/Eci/job/Eci__overlay_ubuntu_bionic_amd64/).
Can we do something to bring those testbeds more closely inline with each other?
Not easily - both systems are designed for very different cases and therefore intentionally using different dependencies (Debian packages vs. latest).
There was a problem hiding this comment.
Nope, I was wrong. python3-pytest-tempdir is not required. Apparently, despite it not being present in the changelog, tmp_path_factory wasn't introduced until v4.3.0 of pytest. I'll propose a fix using an older factory.
For what it's worth, it would help avoid these issues if the "install from source" guide installed Debian packages instead of pip. Otherwise issues like this will continue to crop up, with the CI being the limiting factor. Combined with the fact that these jobs don't seem to be the ones that are typically run on PRs, it means we'll continue to catch these issues only after they land, which isn't ideal.
* create_key test: add missing tests In preparation for future work, add a few tests that are missing from `create_key`, specifically tests that verify the request.cnf, permissions.p7s, governance.p7s, and ecdsaparam. Also rework test module to use module fixtures and multiple smaller test cases instead of one large one. This makes it easier to add more, and makes test failures more meaningful. Signed-off-by: Kyle Fazzari <kyle@canonical.com> * Remove spurious comment Signed-off-by: Kyle Fazzari <kyle@canonical.com> Signed-off-by: ruffsl <roxfoxpox@gmail.com>
In preparation for future work, this PR adds a few tests that are missing from
create_key, specifically tests that verify the request.cnf, permissions.p7s, governance.p7s, and ecdsaparam.Also rework test module to use module fixtures and multiple smaller test cases instead of one large one. This makes it easier to add more, and makes test failures more meaningful.
cc @jacobperron, @emersonknapp