Clar XML output redux by ethomson · Pull Request #4778 · libgit2/libgit2

ethomson · 2018-08-24T10:11:58Z

Reopening a new PR with @tiennou's work in #4741 so that the CI will pick it up.

ethomson · 2018-08-26T16:08:24Z

I didn't really like that it was writing the XML output to the temporary directory (since it opened the file after being sandboxed). This is a result of the aggressively early sandbox, so that it can handle -s test arguments.

So I changed a bit about the way clar handles -s arguments. Previously, in the arg parser, it would just run the tests when it saw a -s and then skip the usual test running that it does.

I made it so that -s actually enables the tests that were specified (disabling everything else). This means that we don't have to set up the sandbox before we parse arguments. So now if we have a -r, we can open the file in the current working directory, then do the sandbox.

I also made -r take an optional argument with the filename. This is because we have five separate libgit2_clar invocations in the CMakeLists.txt and they should have unique filenames. We also need to be able to write them into the build output directory since on the Linux builds (which are run in containers) we need to write to a directory that's mapped back to the host.

So there's some success now. You can see that the although there are linux test failures (because of leaks), they're actually publishing the test results. So that's neat!

But indeed there are still a few problems: this has introduced some leaks, the macOS tests aren't getting picked up, the Windows XML isn't getting written, and there's a failure in one of the Windows builds. But it's getting close!

ethomson · 2018-08-27T00:10:44Z

It turns out that the linux failures were because my original take at refactoring -s support was far too liberal in enabling tests. It enabled whole suites, so the ssh tests that turn on a single test would actually run the entire online::clone suite. Oops.

Originally, I had hoped to avoid any allocations, but that was a silly and unnecessary false optimization. As a result, I refactored the -s parsing so that we now store the values in the argument parsing loop and run them later.

ethomson · 2018-08-27T00:22:29Z

Cool, looks like we've got it working, @tiennou!

ethomson · 2018-08-27T07:37:41Z

I introduced a few memory leaks into clar, which are being detected by valgrind (yay!) but are not failing the build (boo). I'll have to take a look at this before this is ready to .

ethomson · 2018-08-27T07:59:02Z

Okay, I just pushed up a quick test to validate that valgrind was still failing on leaks in the actual product code - it is definitely failing when we "definitely lose" memory. (But not when allocations are "still reachable" at termination.)

This seems reasonable to me but it appears that the macOS and Windows builds are not failing when they detect memory leaks. Since this didn't regress, I want to limit the scope of this PR, so I'll try to address this in a separate one soon.

pks-t · 2018-08-30T11:46:19Z

+
+						_clar_suites[j].enabled = 1;
+						_clar.last_explicit = explicit;
+						break;


I stumbled upon this behaviour before, and didn't quite like it. It also resulted in weird cases with "./libgit2_clar -sodb -sgarbage", where it first runs the odb suite an then complains about the "garbage" suite not being found. So this is a very nice improvement

pks-t · 2018-08-30T11:52:25Z

+{
+	struct clar_report *report;
+	const char *last_suite = NULL;
+	char wd[1024];


pks-t · 2018-08-30T11:53:14Z

+
+	clar_summary_close_tag("testsuites", 0);
+
+	fclose(summary);


One always has to check fclose, as it may produce errors that indicate a file not having been written. Especially so if one uses the buffered interface

pks-t · 2018-08-30T11:54:37Z

+
+		last_suite = report->suite;
+
+		clar_summary_testcase(report->test, "what", 0);


We should also do error checking for these, as we're writing into the FILE pointer. Personally, I'd also vote for making the dependency on the FILE explicit by passing it into the functions, instead of having it implicit via a global. It makes the object's lifetime a lot clearer

Yes, I agree with that.

pks-t · 2018-08-30T11:56:18Z

 OPTION(ENABLE_WERROR			"Enable compilation with -Werror"			OFF)
 OPTION(USE_BUNDLED_ZLIB    		"Use the bundled version of zlib"			OFF)
+   SET(CLAR_XML 			"OFF"					       CACHE STRING
+       "Writes test results in XML format.  One of ON, OFF or the directory to write to.")


Does this really have to be a build configuration? Why can't I just specify this on libgit2_clar's command line?

Edit: oh, because we use it for our CMake-defined tests, that's why. I think we should clarify the documentation a bit, as it sounds like this will enable it for libgit2_clar in general, and not for our CMake tests.

pks-t · 2018-08-30T12:00:50Z

-    inputs:
-      testResultsFiles: 'results_*.xml'
-      searchFolder: '$(Build.BinariesDirectory)'
-      mergeTestResults: true


Lovely, all these repetitive job descriptions were bothering me since the inception. Really sice that you can simply use templates instead

Previously, supplying `-s` to explicitly enable some test(s) would run the tests immediately from the argument parser. This forces us to set up the entire clar environment (for example: sandboxing) before argument parsing takes place. Refactor the behavior of `-s` to add the explicitly chosen tests to a list that is executed later. This untangles the argument parsing from the setup lifecycle, allowing us to use the arguments to perform the setup.

This makes it possible to keep track of every test status (even successful ones), and their errors, if any.

Accept an (optional) value for the summary filename. Continues to default to summary.xml.

Introduce a CLAR_XML option, to run the `ctest` commands with the new `-r` flag to clar. Permitted values are `OFF`, `ON` and a directory to write the XML test results to.

Remove the global summary filename and file pointer; pass them in to the summary functions as needed. Error check the results of buffered I/O calls.

CMake treats backslashes as escape characters; use forward slashes for the XML output path.

Explicitly run from the build directory, not the source. (I was mistaken about the default working directory for VSTS agents.)

Our build YAML is becoming unweildly and full of copy-pasta. Simplify with templates.

Windows lacks %F and %T formats for strftime. Expand them to the year/month/day and hour/minute/second formats, respectively.

ethomson · 2018-09-06T10:19:36Z

Fixed this up per your comments, @pks-t

pks-t · 2018-09-07T10:21:23Z

 /* Core test functions */
 static void
-clar_report_errors(void)
+clar_report(int *i, struct clar_error *error)


By the way, this interface is a bit funny, as it's kind of emulating an iterator via this integer. But in my opinion, using this integer makes code less understandable compared to simply iterating over the linked list of errors ourselves.

Feel free to ignore this comment -- it's not that important after all and can easily be cleaned up later

Huh. Thanks for the detailed review, I had missed that. I agree and will fix it up before I merge it.

And, actually, I decided that I didn't like the way that I fixed it. One of the report functions was still assuming knowledge of the clar structure that a caller also had knowledge of, so I simply passed that in as an arg and I think it's quite a bit nicer. Thanks again for catching this.

pks-t · 2018-09-07T10:32:51Z

Despite the minor comment I have about the weird iterating interface, the PR looks good to me.

By the way -- do we really have 16000 tests? I cannot quite believe that it's that many tests and tend to think the XML output is to blame.

ethomson · 2018-09-08T13:21:47Z

By the way -- do we really have 16000 tests? I cannot quite believe that it's that many tests and tend to think the XML output is to blame.

No - sorry - to clarify, that's an aggregation of all the unit tests run for the build, so it's across all the build hosts. We actually end up executing 23,854 tests in total.

Instead of trying to have a clever iterator pattern that increments the error number, just iterate over errors in the report errors or report all functions as it's easier to reason about in this fashion.

ethomson · 2018-09-09T15:44:18Z

I was playing around with some other things but I'll open a new PR if I decide to pursue them. Shipping this one.

ethomson mentioned this pull request Aug 24, 2018

Clar JUnit XML Support #4741

Closed

ethomson force-pushed the ethomson/clar-xml branch 4 times, most recently from af1594c to 7175af6 Compare August 26, 2018 15:36

ethomson force-pushed the ethomson/clar-xml branch 11 times, most recently from 559ac97 to 4e300c7 Compare August 26, 2018 23:37

ethomson force-pushed the ethomson/clar-xml branch 3 times, most recently from 0a44546 to f653450 Compare August 27, 2018 08:28

pks-t reviewed Aug 30, 2018

View reviewed changes

ethomson force-pushed the ethomson/clar-xml branch from f653450 to 734a6fa Compare September 3, 2018 19:26

ethomson and others added 4 commits September 6, 2018 11:17

Isolate test reports

bf9fc12

This makes it possible to keep track of every test status (even successful ones), and their errors, if any.

Documentation

3a9b963

Barebones JUnit XML output

59f1e47

ethomson added 10 commits September 6, 2018 11:17

clar: don't use a variable named time

dbebcb0

clar: accept a value for the summary filename

baa5c20

Accept an (optional) value for the summary filename. Continues to default to summary.xml.

clar: introduce CLAR_XML option

a2d73f5

Introduce a CLAR_XML option, to run the `ctest` commands with the new `-r` flag to clar. Permitted values are `OFF`, `ON` and a directory to write the XML test results to.

clar: remove globals; error-check fprintf/fclose

b67a93f

Remove the global summary filename and file pointer; pass them in to the summary functions as needed. Error check the results of buffered I/O calls.

ci: write xml during test runs

a84863f

ci: upload test results

bfcbde5

ci: escape xml output path on Windows

f3f2c45

CMake treats backslashes as escape characters; use forward slashes for the XML output path.

ci: explicitly run in the build directory

306875b

Explicitly run from the build directory, not the source. (I was mistaken about the default working directory for VSTS agents.)

ci: use templates for VSTS builds

6b2d8f0

Our build YAML is becoming unweildly and full of copy-pasta. Simplify with templates.

ci: use more compatible strftime formats

e595eeb

Windows lacks %F and %T formats for strftime. Expand them to the year/month/day and hour/minute/second formats, respectively.

ethomson force-pushed the ethomson/clar-xml branch from bc8de73 to e595eeb Compare September 6, 2018 10:19

pks-t reviewed Sep 7, 2018

View reviewed changes

clar: iterate errors in report_all / report_errors

d17e67d

Instead of trying to have a clever iterator pattern that increments the error number, just iterate over errors in the report errors or report all functions as it's easier to reason about in this fashion.

ethomson force-pushed the ethomson/clar-xml branch 6 times, most recently from c99abed to d17e67d Compare September 9, 2018 13:25

ethomson merged commit bc34cb6 into master Sep 9, 2018

ethomson mentioned this pull request Sep 9, 2018

CI: Support multiple executables on a single cmake test #4801

Closed

pks-t added the backport-0.27.6 label Oct 12, 2018

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade nodegit from 0.4.1 to 0.26.4 saurabharch/Breezeblocks#1

Open

snyk-bot mentioned this pull request Apr 22, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 aminatakonate000/Graviton-App#4

Open

snyk-bot mentioned this pull request May 5, 2020

[Snyk] Upgrade nodegit from 0.24.3 to 0.26.5 Barnstorm-Online/ngp-openapi-generator#1

Open


		last_suite = report->suite;

		clar_summary_testcase(report->test, "what", 0);

Uh oh!

Conversation

ethomson commented Aug 24, 2018

Uh oh!

ethomson commented Aug 26, 2018

Uh oh!

ethomson commented Aug 27, 2018

Uh oh!

ethomson commented Aug 27, 2018

Uh oh!

ethomson commented Aug 27, 2018

Uh oh!

ethomson commented Aug 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethomson commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pks-t commented Sep 7, 2018

Uh oh!

ethomson commented Sep 8, 2018

Uh oh!

ethomson commented Sep 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants