RFC: Support arbitrary tagging and selection of testcases. #44
Conversation
|
To allow our other automated tests to run, I need to give our Jenkins instance the following command: Jenkins: ok to test You can disregard this message. I'm looking over the changes and will have a comment shortly. |
| @@ -166,7 +179,10 @@ static void tcase_free(TCase * tc) | |||
| check_list_free(tc->ch_sflst); | |||
| check_list_free(tc->unch_tflst); | |||
| check_list_free(tc->ch_tflst); | |||
|
|
|||
| if (tc->tags) { | |||
There was a problem hiding this comment.
nit pick, but please use the same style which is used elsewhere, namely place the { on the next line, and always use {} even if there is a one line body, for example:
if(condition)
{
do_something();
}
|
I'm leaving comments in the commit itself. As for the appveyor failure, appveyor is used to run builds and tests against Windows compilers. It seems that the check_check unit test program is dying due to a SEGFAULT: https://ci.appveyor.com/project/brarcher46583/check/build/1.0.81/job/c1uktktsivnwjosy
Could not be sure why without debugging it in Windows. I would suspect that if Windows is having an issue then maybe Valgrind may find something interesting in GNU/Linux or OSX. |
|
Ah, the GNU/Linux tests which are run on the Jenkins instance are hitting a double free or corruption issue in glibc. See here for an example.
|
|
You may be able to reproduce the issue locally if you disable using fork(). It appears that the issue manifests when not using fork(), which is the case for Windows and the failed tests on GNU/Linux. Try building with --no-fork, or running a simplified unit test example program with CK_FORK=no in the environment. |
| #include <assert.h> | ||
| #include "check_check.h" | ||
|
|
||
| /* from check_check_master.c */ |
There was a problem hiding this comment.
It may be better to add these tests to the same places where the other tests are located. Try in check_check_selective.c. Check allows a unit test to create suites and run unit tests. This is how Check tests itself. You may need to do this in check_check_selective.c.
What may get you is that all the suites which are added in check_check_main.c have an entry in check_check_master.c in the following table:
static master_test_t master_tests[]
where the entry specifies the suite name, the expected result, and the expected output. For example:
{ "Simple Tests", CK_FAILURE, "This test should fail" },
Each suite is listed in the order they are added, then for each suite the tests are in the order they were added.
Hopefully by adding the tests to the master list some of the duplicated code from check_check_master.c can be avoided.
|
I think that the idea of tagging tests and running tests based on tags has merit. Lets flush this out a bit more and see where it goes. |
|
Interim push to get automated test results - fixed double free crash, wrong use of strtok rather than strtok_r, version number in func comment. |
|
It now appears that when running without fork() that the following test is failing: However when fork() is used it is running the expected number of tests: The issue is probably something simple, but I cannot see it at the moment. As the issue is occurs without using fork(), it is likely something about a data structure being used during a test that is not being reset as it should, but when the test runs with fork() the structure modifications are being discarded when the separate test memory space is discarded. |
|
Hopefully getting closer. I think the main thing not done is undoing the API change to srunner_run (see question on that). Fixed sigsegv. Fixed all the layout convention violations that I can see (is there a check patch like tool I can use ?) Changed layout of tags inside test case to be an array of strings - mostly to avoid the nested use of strtok (since windows doesn't seem to have strtok_r). Provided set/get tags on TCase and got rid of special create function. Added tests to verify set/get. I have also reworked the tag tests in line with what check_check_selective does which has made them much simpler (thanks !). Have not combined into that file as it might make that file rather long - but don't mind. Extended tests to combine include and exclude and to do various 'silly' combinations. Waiting to see if Windows build will now pass ... |
| @@ -65,6 +65,8 @@ struct TCase | |||
| List *unch_tflst; | |||
| List *ch_sflst; | |||
| List *ch_tflst; | |||
| unsigned int num_tags; | |||
| char **tags; | |||
There was a problem hiding this comment.
Check has a list structure available in check_list.c. Consider using that instead of an char**.
There was a problem hiding this comment.
OK - will take a look at the list stuff - wanted to get the self tests all passing reliably first - but touch wood that seems to have happened.
|
Things are coming along, keep it up! I have not gone through the unit tests you proposed yet, but will do that shortly. In addition to the tests you have here, there are a few other types of tests which should be added to ensure proper coverage.
The environment variables maybe should be spelled out instead of abbreviated. The other environment variables summarized here are not abbreviated, except for the CK_ part. Finally, some documentation will need to be added once all it said and done. The main documentation file is located here. Here is the section of the generated documentation which may need to mention the new API calls and environment variables, and here is a summary of the environment variables. |
|
Hi Brandan, |
cdentyou
force-pushed
the
tcase_tags
branch
2 times, most recently
from
5b77afd
to
11b7b1f
Compare
The link I mentioned in the previous comment points to the build output from one of the tests on the Jenkins instance. If you look for the string "are definitely lost" you will find valgrind report stack traces. In addition, you may see lots of SEGV messages in the logs. Those will not affect the test much, but are annoying and are from a bug I introduced unknowingly a few years back. This is now fixed in master, if you want to rebase your changes. |
|
I'm not sure what my Travis failures mean - MacOS builds seem to be silently timing out. I notice that the last build of master seems to have similar problems with the MacOS builds so I'm wondering if this is something I've picked up from master ? I can't seem to find more information than this message - "No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself. It looks like the build is being kicked off but does not produce any output ... |
I'm not sure where to see this. Can you provide a link to an example?
The last several tests runs against master were successful according to this. However, sometimes there are sporadic timeouts on OSX, such as this one 29 days ago.
I've not seen that message, but I wonder if it is from Travis-CI itself, attempting to abort potentially hung jobs after a timeout. The most recently change set did result in a failure on the Jenkins-based tests. I think maybe it expected something to time out that did not. Let me see if it happens again. Jenkins: test this please |
| @@ -65,6 +65,8 @@ struct TCase | |||
| List *unch_tflst; | |||
| List *ch_sflst; | |||
| List *ch_tflst; | |||
| unsigned int num_tags; | |||
There was a problem hiding this comment.
I do not think that this is used anywhere. It is still needed?
|
I think that I like the changes. There was only one small comment I just posted about a variable not being needed anymore. Besides that, things are looking good. As these changes add an environment variable, there is one more test which will need to be added which verifies the environment variable usage. This was mentioned earlier, but may have been lost in the discussion. I'll copy the mention below:
Finally, the environment variables will need to be documented. Here is a description from earlier in the discussion:
|
cdentyou
force-pushed
the
tcase_tags
branch
2 times, most recently
from
95ce18f
to
ebbc272
Compare
|
"As these changes add an environment variable, there is one more test which will need to be added which verifies the environment variable usage. This was mentioned earlier, but may have been lost in the discussion. I'll copy the mention below: ..." I added some tests to check_check_tags.c that are driven by the environment variables in the same way that check_check_selective.c does it for CK_RUN_SUITE/CASE. I took at look at test_output.sh and ex_output.c but it wasn't obvious to me what I needed to do there - they don't seem to do anything related to CK_RUN_SUITE/CASE either. "Finally, some documentation will need to be added once all it said and done. The main documentation file is located here. Here is the section of the generated documentation which may need to mention the new API calls and environment variables, and here is a summary of the environment variables." I have added a section into the documentation which make sense to me - but would appreciate feedback from fresh eyes. And I've added the env vars to the appendix and matched the indexing that I see for CK_RUN_SUITE ... Let me know if there's anything else |
cdentyou
force-pushed
the
tcase_tags
branch
2 times, most recently
from
a41cbda
to
4eb1759
Compare
I see. You are right, that should be sufficient.
Oh, I did not realize there were no tests for CK_RUN_* there. However, as the same coverage is provided elsewhere, no point in duplicating the effort.
I'll take a look |
|
|
||
| It can also be useful to be able to dynamically select or exclude | ||
| groups of tests to be run based on criteria other than the suite or | ||
| testcase name. For example certain test cases can be tagged as |
The test "test_sleep2_fail" is occasionally passing (failing to fail) in the jenkins UT - perhaps 1.6 seconds doesn't give enough leeway ? Making it 1.5 seconds so its in exactly in the middle of sleep1_pass and sleep2_fail.
cdentyou
force-pushed
the
tcase_tags
branch
3 times, most recently
from
aeb0b4a
to
7665d49
Compare
| @@ -974,6 +974,7 @@ easier for the developer to write, run, and analyze tests. | |||
| * Test Fixtures:: | |||
| * Multiple Suites in one SRunner:: | |||
| * Selective Running of Tests:: | |||
| * Selecting Tests Based on Arbitrary Tags:: | |||
There was a problem hiding this comment.
You'll need to add:
* Selecting Tests by Suite or Test Case
here. Otherwise, the index is the following:
...
4.5 Multiple Suites in one SRunner
4.6 Selective Running of Tests
4.6.2 Selecting Tests Based on Arbitrary Tags
4.7 Testing Signal Handling and Exit Values
...
A testcase can optionally have a list of tags associated with it. Srunner can be run with an optional include list of tags and an optional exclude list of tags. These will have the effect of filtering testcases that would otherwise be run.
|
The merge request is accepted. Thanks for pushing this feature through! There is a file in Check which lists the Authors of the project: AUTHORS. You are welcome to add a merge request for this file, adding yourself under the Contributors list. |
|
Hi Branden That's great news ! It's been an absolute pleasure working with you and I'm very grateful for all the work you had to put in to get the code into the right shape. So a huge thank you ! Cris Sent from my iPhone
|
|
No problem. Thanks for your contribution. (: |
Please consider these diffs for adding the ability selectively run tests based on arbitrary sets of tags.
The requirement is to provide a mechanism that will allow us to exclude test cases that need a long time to run from our normal unit test runs which we want to keep fast. We could of course use the existing mechanism for selecting by suite but currently we use suites to subdivide tests based on the functional area being tested so that a user can choose to just run the suite for one area.
In effect we want two orthogonal ways of selecting tests - by speed and by functional area.
We could subdivide the suites into long-running and slow running suites for each functional area but that would force the user (or some external test definition) to list all the suites according to the speed or the functional area that they want to test. Its also possible to envisage further orthogonal criteria that we might want in the future which will mean further lists will have to be maintained.
The alternative that we propose here is to allow an optional list of arbitrary tags (strings) to be provide when a testcase is being added to a suite and then to allow the srunner to take an include and exclude list of tags to allow it to filter testcases. (We would then register come testcases with a "SLOW_RUNNING" tag and run all tests excluding this tag in our normal UT).