Control: tags -1 dh-make-elpa: new Perl-based integration tests Xiyue Deng <[email protected]> writes:
> 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 Friendly ping for comments. It would be good to hear from Sean or David as I plan to work on similar tests on dh-elpa as well, and would like to see whether this is an acceptable approach. (Also renamed to `integration test' as they were not really unit tests.) -- Regards, Xiyue Deng
signature.asc
Description: PGP signature
