Hi Richard, Thanks very much for the review and suggestions! Please see my replies below inline.
Richard Lewis <[email protected]> writes: > On Sat, 17 Jan 2026 at 04:25, Xiyue Deng <[email protected]> wrote: >> >> >> I have been working on a new Perl-based unit tests and I think it's now >> in a usable form. It runs dh-make-elpa in a temp directory and compares >> the contents of its output with the expected ones. > >> [1] https://salsa.debian.org/emacsen-team/dh-make-elpa/-/merge_requests/6 > (i cant review the perl parts in detail though) > > I've used this approach in a couple of other packages, so thought i'd > offer some thoughts based on my experiences > > The pros include > - the test is what the underlying code is actually producing, so you > get confidence that the test is actually testing what a user sees > - you can add tests without needing to understand the code at all, and > the underlying code doesnt need to have any support for "testing" > - it's esp good when you want to know if the output has changed -- i > think this is a good fit for something like dh-make-elpa where > if the output changes it may be unintentional, and checking "by > hand" is tedious > You helped cover several of the design goals of those tests. Thanks! > The cons include > - someone has to keep the test data up-to-date I have written t/utils/check_diff_in_test_cases for this exact reason, which can help update the contents in `expected/' with `--update'. Manually doing that was really a painful experience. I would expect one to have carefully inspected the diffs before committing. > - i think a lot of people in debian commit things before bothering to > test - more git commits fixing test failures > - rebasing branches and MRs that change output can be a HUGE pain This is what the tests are for IMHO. Now we have Salsa CI and autopkgtest to detect the test failures, and who breaks them should fix them. I think it's better than breaking the program unintentionally. > - most test failures will be "the test suite is out of date" rather > than "there is a bug in the package", this can de-sensitize you from > investigating, as once > the "expected" is updated, any issues are hidden. Yes. One needs to carefully inspect that the updated `expected/' contents are intended before committing. This would also promote smaller change per commit, like changing just one aspect and update the tests accordingly in one commit. Changing too many places at once makes it hard to understand and even harder to reason about the diffs and hence should be discouraged. > - you are exposed to changes in other packages (*)see below > - (this may not apply for this package?) you are exposed to > differences in the build environment - it may pass in a local sbuild > but fail on salsa, or in the debian build system, or debusine, or in > stable vs unstable IMHO this should expose a bug either in the package or the tests and should be fixed. So far it passes local sbuild and Salsa CI at least :P > - generally anything that makes the output non-reproducible, like > timestamps are a pain. Indeed. Which is why I generate the diff by excluding any lines that includes a timestamp[1] (I'm lazy :P). Admittedly this is not very robust. I have seen some other software sanitize any timestamp to use "X" to replace each character, which may be a better way to handle this. > - everyone making changes to the package will need to understand how it works Hopefully it's not too complicated to understand: it's basically running `diff' for comparison :) > - in every case ive tried, yu end up needing a way to "filter" the > 'actual' output before comparison to avoid all the unreproducible > bits! > Ack. Similar as the point on timestamp above. > (i think the benefits are more than the costs for a "stable", > slow-moving package with an active maintainer!) > Thanks! > *i did notice that you are hard-coding the contents of a "new git > repos" including all the hook templates (eg > t/data/github_team/repo/git/hooks/commit-msg.sample )- this means > when a new version of git makes a change (even changing the comments > in a template, the test will fail (and maybe block a new git from > migrating?). I dont know if you can instead do a fresh "git init" and > copy the .git into the "expected" directory before doing the > comparison, rather than holding it in the repo? The part of `.git' that matters is the `.git/config' file, and dh-make-elpa uses the `upstream' branch info for generating the `debian/watch' file, so the contents of hooks and info directories don't really matter. As a lazy person, I just did `git init' and edit `.git/config' for the tests. And it turns out those are not needed at all, so I just removed them (and updated the MR). Thanks for pointing this out! [1] https://salsa.debian.org/emacsen-team/dh-make-elpa/-/blob/perl-based-unit-tests/t/lib/TestHelper.pm?ref_type=heads#L125-128 -- Regards, Xiyue Deng
signature.asc
Description: PGP signature
