Fix START_TEST to look like valid C code. #158
Conversation
|
Jenkins: ok to test |
|
The AppVeyor failures are with the 2010 and 2012 Visual Studio releases not supporting inline initialization of structs. Errors like the following are reported: The versions after that, namely 2013, 2015, and 2017, do not find an issue. Check does not claim it supports specific versions of that compiler, only that it supports it. It may be acceptable to drop support for the older versions. The other checker, "default" is a Jenkins job which is failing to clone from the pull request. I'll need to look into that. The change look good to me at first pass. I'll look over it a little more carefully. |
I imagine they don't support initialization with named fields, but if we do initialization by position that might fix it. Instead of: static const TTest __testname ## _ttest = {\
.name = ""# __testname,\
.fn = __testname ## _fn,\
.file = __FILE__,\
.line = __LINE__,\
};\We would use: static const TTest __testname ## _ttest = { ""# __testname, __testname ## _fn, __FILE__, __LINE__ };\Which is OK, not that bad to read... Let me know if you'd like me to push something like that. Otherwise, we could add a requirement on whatever C standard (C99? C11?) specified the ability to include named fields in struct initialization... |
|
Try pushing a change to initialize by position and see if it builds. If it does, we'll go with it.
…On June 14, 2018 5:47:00 PM EDT, Filipe Brandenburger ***@***.***> wrote:
> The AppVeyor failures are with the 2010 and 2012 Visual Studio
releases not supporting inline initialization of structs.
I imagine they don't support initialization with named fields, but if
we do initialization by position that might fix it.
Instead of:
```c
static const TTest __testname ## _ttest = {\
.name = ""# __testname,\
.fn = __testname ## _fn,\
.file = __FILE__,\
.line = __LINE__,\
};\
```
We would use:
```c
static const TTest __testname ## _ttest = { ""# __testname, __testname
## _fn, __FILE__, __LINE__ };\
```
Which is OK, not that bad to read...
Let me know if you'd like me to push something like that.
Otherwise, we could add a requirement on whatever C standard (C99?
C11?) specified the ability to include named fields in struct
initialization...
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
#158 (comment)
|
Done, let's see how that goes... Thanks! |
|
Hm, it still seems unhappy: but I do not see that your second push changed the code. Did your second attempt include the change? |
Instead of exporting the defined name as a bare function, export a struct that has a pointer to the function, but also its name, file and line number where it is defined. Store that information into a new `struct TTest`. After this commit, START_TEST(<testname>) will create three definitions: - <testname>_fn: The actual function; - <testname>_ttest: A `struct TTest` with the information about it; - <testname>: A pointer to <testname>_ttest. Functions `tcase_add_test()` and friends are updated to take a `TTest *` argument rather than a `TFun` and separate name. The runners are updated to find that information inside the linked `tc->ttest`. The call to `tcase_fn_start()` is moved from the defined functions to the runners (both the "fork" and the "nofork" one) which call it just before invoking the test function. A nice side-effect is that END_TEST is now optional, though the empty `#define` is kept for backwards compability. v2: Initialize the struct TTest by position to be compatible with older compilers that do not recognize named fields (e.g. VS 2010, VS 2012.) Tested: - `make check` still passes. - Removing END_TEST from test cases still produces valid code that builds and passes tests.
|
I had changed |
|
That make the Visual Studio build much happier. I think I figured out what broke with the Jenkins job, a change on GitHub's side to deprecate weak crypto that the job was using. That should now be resolved, and I'll rekick the job. Jenkins: retest this please |
|
Jenkins will fetch now, but the updated Jenkins nodes are missing at least one tool. I'll need to have it installed so the various builds and tests can make it through. I've sent a request to have the tool installed, and when it is I'll rekick. |
|
Sounds good, thanks! By the way, do you like the name Cheers! |
|
Hi @brarcher, let me know if you still require an action on my part. (For instance, if Jenkins is now fixed, would you like me to touch my tree, pushing a no-op change like a Thanks! |
|
I got a response from CloudBees, who provides the Jenkins instances for free to FOSS projects. They said it would take over a day for an engineer to fix the VMs to install the missing package, and they did not want to do it. In response to this, I'll need to move all the tests off of Jenkins and onto Travis-CI. When I finish that, I'll merge master into the task branch, which will kick off the tests again. I cannot think of a better term for TTest. Calling it a 'fixture' may be good, such as TestFixture. Though, the name does not mean anything without context. If you want to change TTest to TestFixture (or another name if one comes to mind), besides that you should have nothing to do on your side if the tests which I'll port to Travis-CI pass. If one of the tests do find an issue, I'll poke you. |
|
I've moved the automated tests to Travis-CI here, and am updating the branch to pick up these changes. |
|
All the tests passed, the change is good to go. Thanks for the contribution. If you would like to submit a merge request adding yourself to the AUTHORS file, feel free. |
|
Looks like this change makes it impossible to create a test registry, since now the actual function is not predictable. It also doesn't seem possible to be version agnostic about this breaking change. Do you have a recommendation for how to do this in a way that works before and after this change? |
|
Alternatively, do you have a macro I can |
As of libcheck/check#158 libcheck broke the ability to have a test registry, so we have to resort to code duplication.
Instead of exporting the defined name as a bare function, export a struct that has a pointer to the function, but also its name, file and line number where it is defined.
Store that information into a new
struct TTest.After this commit,
START_TEST(testname)will create three definitions:testname_fn: The actual function;testname_ttest: Astruct TTestwith the information about it;testname: A pointer totestname_ttest.Functions
tcase_add_test()and friends are updated to take aTTest *argument rather than aTFunand separate name. The runners are updated to find that information inside the linkedtc->ttest. The call totcase_fn_start()is moved from the defined functions to the runners (both the "fork" and the "nofork" one) which call it just before invoking the test function.A nice side-effect is that END_TEST is now optional, though the empty
#defineis kept for backwards compability.Tested:
make checkstill passes.END_TESTfrom test cases still produces valid code that builds and passes tests.This PR should address this TODO item.
NOTE: I'm not 100% sure I like the name
TTestbut I wonder what would be a better name for that item... I thought ofTFuncbut that's too similar toTFun. What would you call that? A test fixture? A test function? A test method? Clarifying the terminology would be helpful here...Cheers!
Filipe