Add regression tests for current output

[DanielEScherzer]

No description provided.

[DanielEScherzer]

Apparently Youngsuk_Kim was supposed to be in all caps as YOUNGSUK_KIM in 1.52.0 - even though it wasn't when I reran cargo run --release locally...

[Kobzol]

Thanks! Having tests and an easy way to bless them seems generally helpful.

[xtqqczze]

Is such an extensive regression test necessary, considering the changes in this patch are ~6 MiB?

[Kobzol]

Well, testing the generated HTML is likely the simplest way to check the output of thanks at the moment. The files are very similar and git will likely compress the delta down to a few hundreds KiBs at most, if not less, so I wouldn't worry about that much.

[Kobzol]

Thanks. I think that this is useful, but I also want to hear from @Mark-Simulacrum.

[DanielEScherzer]

Hmm, one disadvantage of the tests is that when the rust-lang/rust mailmap changes, they might start to fail
E.g. I'm pretty sure that the current failure was caused by rust-lang/rust@80777dd, though there aren't any warnings from parsing the mailmap? Asked on the r-l/r PR

I'll wait to hear from Mark-Simulacrum before updating the expected content

We could also add some logic to skip the test on the scheduled website updates and only run it on push and pull_request

[xtqqczze]

The repo is currently 3.9MB on disk, so it’d be great if this didn’t increase it to 10MB just for a single test fixture. Perhaps tests/expected could be stored as a compressed archive in the repo and unpacked during the test instead?

[Kobzol]

The repo is currently 3.9MB on disk, so it’d be great if this didn’t increase it to 10MB just for a single test fixture. Perhaps tests/expected could be stored as a compressed archive in the repo and unpacked during the test instead?

That sounds like unnecessary complexity to me. The size of the test suite on disk won't really increase dramatically in the future, and 10 MiB seems fine. If I do cargo build, the target directory of this project on Linux has almost 500 MiB, so :)

Hmm, one disadvantage of the tests is that when the rust-lang/rust mailmap changes, they might start to fail
E.g. I'm pretty sure that the current failure was caused by rust-lang/rust@80777dd, though there aren't any warnings from parsing the mailmap? Asked on the r-l/r PR

Hmm, yeah, that's a good point. It's almost as if we should have a fixed mailmap.. Or just a test repo, but then that won't exercise all the quirks that occur in rust-lang/rust.

[DanielEScherzer]

Hmm, one disadvantage of the tests is that when the rust-lang/rust mailmap changes, they might start to fail
E.g. I'm pretty sure that the current failure was caused by rust-lang/rust@80777dd, though there aren't any warnings from parsing the mailmap? Asked on the r-l/r PR

Hmm, yeah, that's a good point. It's almost as if we should have a fixed mailmap.. Or just a test repo, but then that won't exercise all the quirks that occur in rust-lang/rust.

We could run the tests with a fixed mailmap:

• [test] manually clone r-l/r to the expected place, replace the mailmap with the known/fixed one, then run cargo to generate output, compare
• [build] just run cargo normally

[DanielEScherzer]

I don't have the rights to rerun failed jobs, so I rebased, the failure should be resolved by now (see #103)