Hi Maxim, thanks a lot for the review, reactions below.
Maxim Cournoyer <[email protected]> writes: > Hi Tomas, > > Tomas Volf <[email protected]> writes: > > [...] > >> From 8357a98e7c0c8690be7fe2903a310a3b93b2ab8f Mon Sep 17 00:00:00 2001 >> From: Tomas Volf <[email protected]> >> Date: Sun, 14 Jul 2024 13:00:14 +0200 >> Subject: [PATCH 2/5] build: test-driver.scm: Output singleton metadata just >> once. >> >> Current implementation printed metadata supposed to be present just once per >> .trs file on the end of each test group. According to the automake's manual >> that is undefined behavior. This commit fixes it by printing that metadata >> just once, after all tests did run. > > Can you show an example of a problematic file, and where in the > documentation it says the metadata should appear only once? Ah, > actually, I think 'make check TESTS=tests/import/elm.scm ' produces a > good example of this: > > # file: tests/import/elm.trs > :test-result: PASS elm->package-name [0.000s] > :test-result: PASS infer-elm-package-name [0.000s] > :global-test-result: PASS > :recheck: no > :copy-in-global-log: no > :test-result: PASS elm->package-name [0.000s] > :test-result: PASS infer-elm-package-name [0.000s] > :global-test-result: PASS > :recheck: no > :copy-in-global-log: no > :test-result: PASS elm->package-name [0.000s] > :test-result: PASS infer-elm-package-name [0.000s] > :global-test-result: PASS > :recheck: no > :copy-in-global-log: no > :test-result: PASS elm->package-name [0.000s] > :test-result: PASS infer-elm-package-name [0.000s] > :global-test-result: PASS > :recheck: no > [...] > > Also, could you please reference the section of the manual which you say > says it's undefined behavior? I couldn't find it; though it does seems > unsatisfactory/problematic! For sure, this is documented in: (automake)Log files generation and test results recording Quoting the relevant parts: What happens when two or more ‘:recheck:’ fields are present in the same ‘.trs’ file is undefined behavior. What happens when two or more ‘:copy-in-global-log:’ fields are present in the same ‘.trs’ file is undefined behavior. What happens when two or more ‘:test-global-result:’ fields are present in the same ‘.trs’ file is undefined behavior. > >> Since there is no built-in hook that could be used for >> that (test-runner-on-final runs on *each* outermost test-end), I introduced >> new `finalize' procedure that need to be called by the user. Possibly not >> the >> most elegant solution, but since we are the only user, it works fine and >> produces actually valid .trs file. > > What do you mean by "test-runner-on-final runs on *each* outermost > test-end"? I don't understand how there could be more than one outermost > test-end? The following is a valid test files, and it has 2 outermost test-ends. --8<---------------cut here---------------start------------->8--- (test-begin "foo") (test-begin "xx") (test-end) ;not an outermost (test-end) ;first outermost (test-begin "bar") (test-end) ;second outermost --8<---------------cut here---------------end--------------->8--- The SRFI-64 mandates that test-runner-on-final will, on this specific file, be invoked twice. > >> * build-aux/test-driver.scm (test-runner-gnu): Define new procedure >> `finalize' >> and return it together with the runner. Do not call >> test-runner-on-group-end!. >> (main): Call the `finalize' after all tests are done. > > Applied, and tested with the same example above. It seems to work > great, thank you. > > By the way, I had to come up with a small workaround for your new SRFI > 64 implementation that landed in Guile 3.0.11 (now available in Guix); > see commit 5fada9a751f as well as upstream issue > <https://codeberg.org/guile/guile/issues/133>. Thank you for the ping. It is some time since I implemented it, so my memory is bit fuzzy, but looking at the code and SRFI, I do not believe this is a bug. Not having a test on the run list is not the same as skipping it. The specification says the following for the `Result kind', emphasis mine: *Running* a test may yield one of the following status symbols: When the test is not on the run list (== list of tests to run), it was never *ran* (== the action of *running* was never performed on it), so the kind is not supposed to be set. At least per my reading of the specification. If the kind would be set to 'skip even in the case of the test was not on the run list, there would be no way to tell whether test was skipped due to skips, or not ran due to not being requested via run list, which would quite limit the options of custom test runners (they need to be able to tell the difference). I would appreciate if you could add the above to the Guile's bug report, I do not have Codeberg account, so I cannot react there. Tomas -- There are only two hard things in Computer Science: cache invalidation, naming things and off-by-one errors.
