rnpgp / rnp

RNP: high performance C++ OpenPGP library used by Mozilla Thunderbird
https://www.rnpgp.org
Other
201 stars 55 forks source link

Restructure src/cmocka #304

Closed dewyatt closed 7 years ago

dewyatt commented 7 years ago

Description

I was thinking we can improve the file structure and naming in src/cmocka. We have all source files prefixed with rnp_tests_ which is a bit redundant. Also the directory name cmocka seems unnecessarily implementation-specific and could probably just be tests (or tests/cmocka)?

I was thinking something along these lines would be a minor improvement:

$ tree rnp/src/tests/
rnp/src/tests/
├── cipher.c
├── exportkey.c
├── generatekey.c
├── Makefile.am
├── rnp_tests.c
├── rnp_tests.h
├── support.c
└── support.h

0 directories, 8 files

In the future it may make sense to split the tests up more and maybe add some subdirs that test specific components, etc.

But I think the above is at least a small step towards something slightly more organized.

EDIT: typo

ronaldtse commented 7 years ago

Can I give three thumbs up for this? :-)

ni4 commented 7 years ago

I can do this when will be updating cmocka with CLI tests. Probably we should also remove current contents of tests directory, they seems a bit outdated? Another idea - some tests worth running multiple times (like low-level ecdsa/eddsa key generation/signing), probably cmocka allow it. Or just update tests themselves.

dewyatt commented 7 years ago

some tests worth running multiple times

@ni4 I believe @catap added some support for this in 6d550494ce503d2ee8c555c11349d740df336b95. Looking at it now though, I think it needs some work. I would expect the loop to just call cmocka_run_group_tests multiple times and do an early exit if ret != 0. It seems like the result isn't currently used. Anyways, maybe you can build on that.

dewyatt commented 7 years ago

I can do this when will be updating cmocka with CLI tests.

@ni4 Also I think it would be helpful if you could do this in a separate PR, that ought to be quicker and easier to review.

Probably we should also remove current contents of tests directory, they seems a bit outdated?

I think there was some discussion about this and @ronaldtse had some input I can't recall.

ni4 commented 7 years ago

@dewyatt Sure. Previous two PRs were huge and messy due to large amount of breaking changes, which then were merged together with new live changes which used older codebase. I don't like such PRs as well.

zgyarmati commented 7 years ago

As a sidenote here, but maybe it worth a separate ticket: it also would be nice from the packaging point of view to get all of tests (both cmocka and CLI tests) integrated into automake and invoked by a single call of make check.

ronaldtse commented 7 years ago

Agree to make check!

@ni4 my input to removing tests / tst directories, is a resounding yes as long as we have a CLI tester. You've done it in Python, right? If we can merge that it'll be fantastic.

ni4 commented 7 years ago

@ronaldtse Yeah, I started in Python for performance tests, and then will add functionality tests. So will merge that in once ready

ronaldtse commented 7 years ago

Looking forward to it, thanks @ni4 !

ni4 commented 7 years ago

Fixed with #314