Hi,

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.  This can cover more
behaviors of dh-make-elpa in the tests.

The changes are kind of big, so I would like to direct you to the
proposal in this MR[1].  A high-level overview can be found at
`t/README.org'.

I hope this can prove to be useful, and the idea can be generalized for
dh-elpa later.

Reviews and suggestions welcome!

[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

The cons include
- someone has to keep the test data up-to-date
- 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
- 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.
- 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
- generally anything that makes the output non-reproducible, like
timestamps are a pain.
- everyone making changes to the package will need to understand how it works
- in every case ive tried, yu end up needing a way to "filter" the
'actual' output before comparison to avoid all the unreproducible
bits!

(i think the benefits are more than the costs for a "stable",
slow-moving package with an active maintainer!)

*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?

Hi Richard,

Thanks very much for the review and suggestions!  Please see my replies
below inline.

Richard Lewis <richard.lewis.debian@googlemail.com> writes:

You helped cover several of the design goals of those tests.  Thanks!

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.

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.

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.

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

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.
`diff' for comparison :)

Ack.  Similar as the point on timestamp above.

Thanks!

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

Xiyue Deng <manphiz@gmail.com> writes:

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.)