Conversation
test/example.c
Outdated
| #define STRING_BUFFER_SIZE 100 | ||
| char string_buffer[STRING_BUFFER_SIZE]; | ||
|
|
||
| #define RETURN_ON_ERROR_WITH_MESSAGE(_error_code, _message, _result) { \ |
There was a problem hiding this comment.
Couldn't this declare a local test_result and thereby keep the same name and interface as CHECK_ERR?
There was a problem hiding this comment.
Sort of. We could create a new scope block in the macro.
C89 requires variables be declared at the top of the scope block. Without creating a new scope block, I needed to pass in the already-created variable. But a new scope block & local test_result makes sense.
That said, it would be a significant-enough difference from CHECK_ERR I think. CHECK_ERR would always exit(1) on error. This will always return. So maybe a different name should still be used? Especially since there are other macros for the other cases.
What do you think?
There was a problem hiding this comment.
I'm a fan of keeping the diff size small. It's kosher IMHO to redefine the meaning of CHECK_ERR slightly; the name never did actually promise to abort.
Check out the branch I sent, I think it's tider...
There was a problem hiding this comment.
I looked at the branch.
I don't mind repurposing CHECK_ERR. However, it looks odd to mix CHECK_ERR and RETURN_WITH_MESSAGE / RETURN_WITH_EXTENDED_MESSAGE.
Maybe those could be renamed to SKIP_ERR_CHECK / CHECK_ERR_WITH_EXTENDED_MESSAGE or the likes?
Also, your branch needs to introduce a scope block to remain compatible with C89.
There was a problem hiding this comment.
I like your new suggested names, except I'm irrationally attached to the name CHECK_ERR and smaller diffs. If there will be no usage of the old version of the macro, why not steal its name?
Also, not sure which scope I'm missing, gcc -std=c89 seems to pass on my branch...?
There was a problem hiding this comment.
I meant keeping the CHECK_ERR name and replacing RETURN_WITH_MESSAGE with SKIP_ERR_CHECK and replacing RETURN_WITH_EXTENDED_MESSAGE (now gone) with CHECK_ERR_WITH_EXTENDED_MESSAGE.
Now that RETURN_WITH_EXTENDED_MESSAGE is gone, maybe we can go with just CHECK_ERR and DONT_CHECK_ERR?
The scopes you are missing are in the macros
RETURN_SUCCESS
RETURN_SUCCESS_WITH_MESSAGE
RETURN_SUCCESS_WITH_EXTENDED_MESSAGE
It looks like -std=c89 or -ansi only disable certain things, which doesn't include the variables-at-top-of-scope-block requirement:
https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/C-Dialect-Options.html
It looks like -pedantic may hit it ("it finds some non-ISO practices, but not all"):
https://gcc.gnu.org/onlinedocs/gcc-4.2.4/gcc/Warning-Options.html#Warning-Options
There was a problem hiding this comment.
(I meant -std=c89 and -pedantic may hit it. Not -pedantic alone.)
There was a problem hiding this comment.
Thanks, adding -pedantic did find one in test_large_inflate, but the three macros you're looking at do declare scopes, look again :-)
I kind of like the RETURN_WITH_MESSAGE name for something that just returns with a message and doesn't check... although maybe it should be RETURN_FAILURE_WITH_MESSAGE (vs RETURN_SUCCESS_WITH_MESSAGE)?
There was a problem hiding this comment.
Oh wait. Right.
I blanked on macros being weird and thought the outer {} didn't count as scope. I'll update this CL to remove the extra {}s I added which we don't need.
That said, I'm not sure I see the issue in test_large_inflate.
There was a problem hiding this comment.
I like your idea of RETURN_FAILURE_WITH_MESSAGE and SUCCESS.
In fact, we can leave out the _WITH_MESSAGE. Now I think we're landing on some good names. Glad you pushed for CHECK_ERR :D
I uploaded a commit with CHECK_ERR, and RETURN_SUCCESS, RETURN_FAILURE.
test/example.c
Outdated
| int error_code; /* error code if success is FAILED_WITH_ERROR_CODE */ | ||
| int line_number; | ||
| z_const char* message; | ||
| z_const char* extended_message; |
There was a problem hiding this comment.
gzerror's return type is const char *, not z_const char *, leading to warnings?
There was a problem hiding this comment.
I think I misunderstood the purpose of z_const. I think you're right. I'll make that change.
|
Could you elaborate on bypassing ctest? |
test/example.c
Outdated
| RETURN_WITH_MESSAGE("bad uncompress\n", result); | ||
| RETURN_WITH_MESSAGE("bad uncompress\n"); | ||
| } else { | ||
| test_result result; |
There was a problem hiding this comment.
Nicer. This happens quite a few times, though; want to add a RETURN_SUCCESS() etc. to shorten the
common case? I did that in https://github.com/dankegel/zlib/blob/tidier-maybe/test/example.c#L74
There was a problem hiding this comment.
Good idea. I'll do that.
In fact, looking at those cases makes me realize something. I could add 2 macros for the message / extended_message situation. Or I could just pass NULL as the extended message and use 1 macro.
This same idea could be applied to the existing macros.
So I'll do both of those things.
|
It's just my personal bias towards using same test command on cloud as one does locally. I don't think it's that important. |
|
I like your idea of keeping as much as possible the same between local & CI testing. The existing local tests just print to stdout, which isn't an option for the CI testing. So the two must diverge for now. (We can later add proper tests & test harnesses, which could help keep the two uses similar. But I figure that warrants a separate pull request since it would be an independent project.) Maybe we could have AppVeyor use ctest to keep things more similar. Is there a way to pass a command line parameter to ctest? I checked https://cmake.org/cmake/help/latest/manual/ctest.1.html but don't see anything. |
|
googletest obeys both a commandline argument and an environment variable for exactly this situation; passing ZLIBTEST_JUNIT=foo could do the same thing as --junit=foo. Incidentally, that option should probably add the junit output file without disabling the normal output. That keeps the remote and local cases closer IMHO. Now that I look at it, why does the non-junit case terminate on first error? Might want to make that a separate option. I know it was the old behavior, but... |
|
uh why appveyor? Does github actions not support C? I thought github actions supports windows-latest, ubuntu-latest, and also macosx-latest While Windows got VS2019. |
|
Maybe not a bad idea for the --junit command line parameter to add to the existing output. The problem was I wanted to preserve the old behavior (which called exit(1)). I suppose I could do something like if (!is_junit) but then I need to pass that into the test functions, which changes their interface and feels messy. In the end, we should be using a different test harness anyway. So I figured for now this was the way to go. @AraHaan When we first looked into this, GitHub Actions weren't an option because of security. My understanding is at the time, relying on an Action maintained by someone else opened up the possibility of an Action committing code into zlib that we didn't want. Since that original glance, there is apparently a way to provide your own Actions and not have this security issue. I'll look into GitHub Actions as an option again. I feel like it might be the better option since it'll integrate more nicely. |
|
Re other test harness - are there any that support being built with a K & R C compiler? is_junit should probably be a global anyway, at which point it's easy to add g_abort_on_arror. |
test/example.c
Outdated
| return result; \ | ||
| } | ||
|
|
||
| void handle_test_results(FILE* output, test_result result, z_const char* testcase_name, int is_junit_output, int* failed_test_count) { |
There was a problem hiding this comment.
This doesn't look like legal K&R C.
|
I'm not sure about the K&R requirement for testing. That's probably up for discussion among the zlib contributors / maintainer. I suspect it would be ideal but there might not actually be any. At which point, I imagine we'll all weigh how much we want to write our own vs. give up that requirement. The various CI integrations have different types of support for test harnesses. JUnit format seemed to be common enough, so that is what I went with here. It looks like Unity doesn't have support for the common types, but does allow you to control output. That would be an option, I suppose. It is basically what this pull request does. Either way, that's probably a follow-up pull request. Even if we make is_junit and g_abort_on_error globals, we still need a way to pass --junit to ctest. I saw your example that bakes the parameter into ctest, but doing that loses the old behavior. In either case, I'm not terribly worried because I suspect we'll want to either use a proper test harness (at which point we don't want to keep the old behavior) or write a more robust output formatter. Maybe we should cross that bridge when we get there? |
|
In zlib-ng we use GitHub Actions. You might want to take a look at that. |
test/example.c
Outdated
| if (!is_junit_output) { | ||
| if (result.message != NULL) { | ||
| if (result.extended_message != NULL) { | ||
| fprintf(output, "%s%s\n", result.message, result.extended_message); |
There was a problem hiding this comment.
whoopsie. should be stderr.
|
@nmoinvaz Right. Will do. :) |
|
So, with https://github.com/dankegel/zlib/tree/env-envy, you can pass --junit foo.xml to example by setting ZLIBTEST_JUNIT="$( Wait, that's kinda dumb, lemme make sure it works with linux, where I need to run both example and example64. Kinda tempting to make the env var prefix ZTEST_ and the commandline option prefix --ztest_. |
|
Good idea to get the environment variables. I'll try that. I'm not sure I like the idea of moving things to globals though. I could imagine a situation where we need to use two CI providers (maybe to have an external logging site?). If we do globals, we need to run the tests twice. If we use formatters, we run the tests once and produce two results. I don't even like that I used a global for the sprintfs. I needed the lifetime to live beyond the function (so couldn't be local). That said, I'm happy to do it if people like it. (BTW Dan, I think you can fork my repo and submit pull requests there to get your contributions counted as part of this PR.) |
|
Not sure I understand the global vs. formatter thing. I'm just running once, and everybody's happy, I think. Pull request coming... |
| result.error_code = _error_code; \ | ||
| result.line_number = __LINE__; \ | ||
| result.message = _message; \ | ||
| return result; \ |
There was a problem hiding this comment.
extended_message uninitialized, unpredictable results.
There was a problem hiding this comment.
The handler for FAILED_WITH_ERROR_CODE doesn't try to access the extended message. See line 80.
There was a problem hiding this comment.
Next guy might reference it. cough
There was a problem hiding this comment.
Agreed. It is like a time bomb / trap for the next person to fall into.
I'll initialize all the variables.
|
Having forked madler/zlib, I can't fork yours :-( https://github.com/dankegel/zlib/tree/develop adds a --force_fail option which injects a fault to test the annotations. (It also rejiggers the xml to be closer to what I was generating before, because I wasn't seeing annotations in github; still not quite sure about it, but at least clicking on the head commit in my branch shows nice test failure annotations on the responsible source line.) |
|
https://github.com/dankegel/zlib/tree/develop has been tidied up (and, since I couldn't submit it as a pull request against Chris's branch, submitted for review as #495 ). It includes cirrus-ci builds, adds options/env vars (--force_fail, --fail_fast) that are useful when doing ci, and has a couple minor cleanups, including better failure annotations on github. https://github.com/dankegel/zlib/tree/moreci includes examples of using other CI infrastructures (travis, drone) that reach more CPU types. |
|
I'm curious: is K & R compatibility a requirement for zlib? |
|
For key infrastructure, backwards compatibility is pretty important, and it's customary to give a year or so's warning before dropping it, so developers can prepare or object. zlib's bunkmate libpng seems to have warned in 2011 that K&R support was going away, Amazingly, there may still be a few K&R-only compilers around, e.g. HP/UX's bundled compiler: So FWIW if zlib has not yet warned that K&R support is going away, I'd suggest making the announcement now but staying K&R compatible through the end of the year. The alternate viewpoint (kill K&R with fire) has merit, though. |
|
I noticed there is Cirrus CI, Drone CI, Travis CI, and AppVeyor. Are you adding all the CI platforms? Would you be open to GItHub Actions as well or is AppVeyor settled? It is possible to use CMake and qemu to run the emulated tests for platforms that are not natively supported by one CI platform or another. |
|
I doubt we'll be adding all those CI platforms, they're just examples. I'm aware of the qemu approach and am looking at it now. |
Very much agree with that statement. I guess even knowing that zlib is used in some crazy environments, I still didn't expect there would still be environments that require K & R C in 2020. |
test/example.c
Outdated
|
|
||
| void handle_test_results OF((FILE* output, test_result result, z_const char* testcase_name, int is_junit_output, int* failed_test_count)); | ||
|
|
||
| void handle_test_results(output, result, testcase_name, is_junit_output, failed_test_count) |
There was a problem hiding this comment.
I found handle_test_results fragile in that it was hard to change the XML format because the two output formats and the state management were all mixed together. (Hence the commit in my pull request to refactor it.)
There was a problem hiding this comment.
That's what I meant by "formatter" earlier.
We could produce multiple formats of output rather than run the tests multiple times.
That said, I think you're right, that I should have two separate functions for the two formats.
I'll do that soon.
|
LGTM except for the readability problem in handle_test_results, and the somewhat nonstandard name of the tooling directory. |
| @@ -0,0 +1,32 @@ | |||
| version: 1.2.11.1.{build} | |||
There was a problem hiding this comment.
Other projects seem to be converging on 'ci' rather than 'tooling', e.g. yesterday's commit pnggroup/libpng@3676fd3
There was a problem hiding this comment.
I've moved the directory from tooling/appveyor/ to ci/.
I originally thought a tooling/ directory would be good because it makes room for other tools such as static analyzers without muddying the root directory. However, if/when we get to that bridge we can cross it. If ci/ is more comfortable, I'm happy to go with that.
| # &"$($env:APPVEYOR_BUILD_FOLDER)\$($env:CONFIGURATION)\example.exe" | ||
| test_script: | ||
| - ps: | | ||
| $env:ZLIB_JUNIT_OUTPUT_FILE = "$($env:APPVEYOR_BUILD_FOLDER)\$($env:CONFIGURATION)\junit-results.xml" |
There was a problem hiding this comment.
Alas, this assumes that cmake only runs one test executable. This fails on Linux, where cmake runs both example and example64; the junit output of both must be uploaded. Perhaps we need a cmake option to turn on junit output, and use that to control passing the --junit flag to the test programs. I can give that a shot.
| } | ||
| } | ||
| if (output_file_path) { | ||
| fprintf(stdout, "output path is %s", output_file_path); |
| if (output_file_path) { | ||
| fprintf(stdout, "output path is %s", output_file_path); | ||
| is_junit_output = 1; | ||
| output = fopen(output_file_path, "w+"); |
There was a problem hiding this comment.
The program never reads from the output file pointer; why the + ?
|
|
||
| /* =========================================================================== | ||
| * Usage: example [output.gz [input.gz]] | ||
| * Usage: example [--junit results.xml] [output.gz [input.gz]] |
There was a problem hiding this comment.
as long as we're touching this line, does example.c really take an input filename?
| is_junit_output = 1; | ||
| output = fopen(output_file_path, "w+"); | ||
| if (!output) { | ||
| fprintf(stderr, "Could not open junit file"); |
There was a problem hiding this comment.
missing newline here, too
|
OK, my current followons to this pull request are now visible at #500 |
|
I haven't spoken here in a little over a week. I wanted to say something to show that I haven't forgotten or abandoned it. I'll respond once I have time to form an intelligent response. I've been busy with other things. But I will reply when I can. |
|
@ProgramMax if you need help let me know and I can try to put something together. |
|
If the group decides to try github actions, I'm happy to help with that,
too.
…On Thu, Jun 4, 2020 at 8:25 PM Nathan Moinvaziri ***@***.***> wrote:
@ProgramMax <https://github.com/ProgramMax> if you need help let me know
and I can try to put something together.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#492 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABJXYED6S7XMROW3CRXZTLRVBQTJANCNFSM4NDRIEFQ>
.
|
|
Here are two workflows for GitHub Actions you can use: https://github.com/nmoinvaz/zlib/blob/improvements/ci-cmake/.github/workflows/cmake.yml https://github.com/nmoinvaz/zlib/blob/improvements/ci-cmake/.github/workflows/configure.yml They are created using latest master branch of this repository and can be expanded if anyone wants to add additional configurations. |
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
|
I have created a PR #506 for GHA changes so they can be more easily referenced. Take from it what you need. I also added a workflow for OSS-Fuzz, so that each pull request is fuzzed. |
|
Mark, your appveyor builds of this pull request and its ilk are failing at the moment; I think this is how to fix them:
|
|
@madler: What do you think? |
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
…mmit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
On each commit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
On each commit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
On each commit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
On each commit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
On each commit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
On each commit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
On each commit these workflows will be run to verify that project generation, source file compilation, and test cases run successfully. On each pull request a workflow will be run to verify that all fuzzing tests pass successfully. madler#492
|
I think we have this covered now with github actions. |
To make it easier to accept contributions to zlib we should establish automated testing.
This commit uses the existing test/example.c testing. It connects those tests to AppVeyor, allowing AppVeyor to mark commits as passing or failing the tests. It also automatically runs tests when a pull request is submitted or updated.
There is some room for future expansion enabled, such as tracking the file and line of a failing test. This is thanks to @dankegel, who also got this working on CI providers other than AppVeyor.
In order for this to land properly in madler/zlib, the README.md (the renamed and reformatted README file) must be updated with new [appveyor-shield] and [appveyor-link] links. These can be obtained by @madler once they enable AppVeyor on madler/zlib.