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)

[Mark-Simulacrum]

I think mailmap updates breaking this seems like a real concern. Maybe we can leave the contents here with either fixed mailmap or no mailmap at all? Though I believe https://github.com/rust-lang/thanks/blob/master/src/reviewers.rs changes will also require updates.

re:repository size, one of the concerns I'd have is that overtime each change will add a new copy of all the output to the git history, which means we'd grow potentially rapidly. One option could be that we just store hashes of the output: if you break it, it should be possible to generate the old and the new output locally and do a proper diff. But that would prevent meaningful growth of the actual repository (hashes are tiny, after all). If we did that then we could probably avoid fixed mailmaps too? There's not that much churn on the mailmap in practice I think.

[DanielEScherzer]

I think mailmap updates breaking this seems like a real concern. Maybe we can leave the contents here with either fixed mailmap or no mailmap at all? Though I believe https://github.com/rust-lang/thanks/blob/master/src/reviewers.rs changes will also require updates.

I think having no mailmap at all would mean that we lose the testing of the merging of mailmap entries, which we might not want to lose

But having a fixed mailmap shouldn't be an issue - since the mailmap is fetched from the current HEAD commit, we would just have the tests check out a specific older revision of the r-l/r repo to run the tests on

Though I believe https://github.com/rust-lang/thanks/blob/master/src/reviewers.rs changes will also require updates.

Those changes would be to this repo and so my thinking was that it wouldn't be problematic to update the tests at the same time

re:repository size, one of the concerns I'd have is that overtime each change will add a new copy of all the output to the git history, which means we'd grow potentially rapidly.

We don't actually need to add each new release (or even all of the historical ones, I just did that since they were all available)

One option could be that we just store hashes of the output: if you break it, it should be possible to generate the old and the new output locally and do a proper diff. But that would prevent meaningful growth of the actual repository (hashes are tiny, after all). If we did that then we could probably avoid fixed mailmaps too? There's not that much churn on the mailmap in practice I think.

That would work, but it means that the process of reviewing changes that change the output would be a bit harder. What if instead the list of contributors was re-formatted in the tests to remove a bunch of the HTML? E.g. instead of (looking at 0.1.0.html)

            <tbody>
                <tr>
                    <td class="bn">1</td>
                    <td class="bn">Brian Anderson</td>
                    <td class="bn">1958</td>
                </tr>
                <tr>
                    <td class="bn">2</td>
                    <td class="bn">Graydon Hoare</td>
                    <td class="bn">1526</td>
                </tr>
...

we had

            <tbody>
1 |  Brian Anderson | 1958
2 | Graydon Hoare | 1526
...

it wouldn't be correct HTML, but it would make output changes easy to review (perhaps even easier than they are with the HTML) and reduce the size needed to store historical releases (saving ~155 characters per entry in the table)

[Mark-Simulacrum]

it wouldn't be correct HTML, but it would make output changes easy to review (perhaps even easier than they are with the HTML) and reduce the size needed to store historical releases (saving ~155 characters per entry in the table)

I believe git compresses within the .git storage, so probably those size wins would be fairly minor. I think producing a reasonable diff could be done by comparing against the published artifacts for the most part? Those are presumably 1:1 with the local generated HTML?

I don't have a strong opinion here either way though.

[DanielEScherzer]

it wouldn't be correct HTML, but it would make output changes easy to review (perhaps even easier than they are with the HTML) and reduce the size needed to store historical releases (saving ~155 characters per entry in the table)

I believe git compresses within the .git storage, so probably those size wins would be fairly minor. I think producing a reasonable diff could be done by comparing against the published artifacts for the most part? Those are presumably 1:1 with the local generated HTML?

I don't have a strong opinion here either way though.

We could check against the published artifacts, but that requires a network connection and fetching them individually

I've updated this to do the testing against a pinned version of the mailmap (see #104 to confirm that the mailmap is actually read from the pinned version rather than the current latest - by pinning to an older version, the tests start to fail)

---

Before applying that cleanup, du -sh tests/expected/ returned 6.2M, afterwards 1.4M, so I think the simplifications help and I've applied them. We could further simplify things by replacing the <nav class="flex flex-row justify-center justify-end-l items-center flex-wrap ph2 pl3-ns pr4-ns"> and parts of the <head> tag, but I think this is now small enough

[Kobzol]

Sorry for the delay, I didn't have much time for reviews in the past few months. Diffing against the published artifacts sounds like it might work, though once it changes you can't easily go back in history and diff against older versions.

The main thing that we want to test is:

1. Names of the contributors
2. Their contribution counts

For specific historical releases (doesn't even have to be all of them). We don't really need to check any HTML output, that's not so interesting for us to have a test for, I think. Is there a simple way to hook into the current rendering logic to produce e.g. a MD file or a JSON file, as you suggested, and store only that to git, and diff that?

[DanielEScherzer]

Sorry for the delay, I didn't have much time for reviews in the past few months. Diffing against the published artifacts sounds like it might work, though once it changes you can't easily go back in history and diff against older versions.

The main thing that we want to test is:

1. Names of the contributors
2. Their contribution counts

For specific historical releases (doesn't even have to be all of them). We don't really need to check any HTML output, that's not so interesting for us to have a test for, I think. Is there a simple way to hook into the current rendering logic to produce e.g. a MD file or a JSON file, as you suggested, and store only that to git, and diff that?

My worry is that the more logic we add to make the tests simpler, the further they stray from the actual output
The easiest option would be to just replace templates/stats.hbs with something like (so that we still test the release title, in progress, counting, etc.):

{{ release_title }} Contributors

{{#if in_progress }}
We have had {{ count }} individuals contribute to {{ release }} so far.
{{else}}
We have had {{ count }} individuals contribute to {{ release }}.
{{/if}}

Rank | Name | Contributions
{{#each scores as |score| }}
{{score.rank}} | {{score.author}} | {{score.commits}}
{{/each}}

and that way we don't have all of the other parts of the HTML, and don't need to modify the production code. Though personally I would prefer to do the modifications in the test assert_file_content_matches() rather than by switching out the template

As for reducing the number of releases to test against, #37 (comment) reported that there were only unexpected changes in 1 release (1.34.0) so if we don't test every release we run the risk of an untested release page having unexpected changes

[DanielEScherzer]

Before additional simplification:

root@ea7197ba2d80:/rust/thanks# du -sh tests/expected/
1.4M    tests/expected/

after removing the sections: 1.3M
after then removing the

sections: 1.2M
removing the remaining HTML bits: 1.1M

So we can drop the other stuff, giving us expected files like

Rust 1.93.1 Contributors
We have had 7 individuals contribute to 1.93.1.
Thank you so much!

1 | Josh Stone | 3
2 | Jieyou Xu | 2
3 | Alex Crichton | 1
3 | Guillaume Gomez | 1
3 | Jonathan Brouwer | 1
3 | Linshu Yang | 1
3 | bors | 1

but it wouldn't save much space on disk. Let me know if I should apply those simplifications

[Kobzol]

Those sizes seems surprising to me, the HTML parts should be a large fraction of the text, so removing them should save much more space, or at least that's what I would expect (this is without compression, right?).

[DanielEScherzer]

Those sizes seems surprising to me, the HTML parts should be a large fraction of the text, so removing them should save much more space, or at least that's what I would expect (this is without compression, right?).

Correct, this is without compression - given that I already removed the table formatting, the vast majority of the remaining text is just the lists of contributors - 89e4a99 already did most of the size reduction from the tables

[Kobzol]

I took another look at this. I don't like that we are sort-of testing the HTML, because we aren't really, but it looks like we are 😆 We don't need HTML tests, we just want to ensure that the author scores are correct. In order to make that easier to test, I opened #105, which adds a CSV output mode. That will output only the information that we want to test, which should make this easier.

The mailmap thing is still not fully resolved, because after a mailmap change, probably all (or most of) the files can change. We should also somehow store in the output directory on which rustc commit was the output based, so that it can be reproduced locally, and not just on CI. Ideally, thanks should take the rustc commit as an optional input parameter, so that you can locally reproduce its output for the given rustc SHA.

[DanielEScherzer]

The mailmap thing is still not fully resolved, because after a mailmap change, probably all (or most of) the files can change. We should also somehow store in the output directory on which rustc commit was the output based, so that it can be reproduced locally, and not just on CI. Ideally, thanks should take the rustc commit as an optional input parameter, so that you can locally reproduce its output for the given rustc SHA.

If you look at the GitHub actions setup, it is pinned to a specific rustc commit specifically so that the mailmap doesn't change

[DanielEScherzer]

Rebased, squashed, switched to CSV output mode

[DanielEScherzer]

-235,Stefan Schindler,dns2utf8@users.noreply.github.com,1
 235,Stefan Schindler,dns2utf8@estada.ch,1
+235,Stefan Schindler,dns2utf8@users.noreply.github.com,1

looks like the order for CSV isn't deterministic? Or things were just ordered wrong for me locally?

[DanielEScherzer]

-250,Frederick Zhang,Frederick888@Tsundere.moe,1
+250,Frederick Zhang,frederick888@tsundere.moe,1

in 1.25.0 - but last time, the failure was in 1.91.0, so it looks like the email selection is also not deterministic?
Also, with just one commit, there should only be one email?
Lets run the tests again (after fixing rustfmt)

[DanielEScherzer]

now the failure is from the same user but on 1.22.0

-117,Frederick Zhang,frederick888@tsundere.moe,5
+117,Frederick Zhang,Frederick888@Tsundere.moe,5

@Kobzol any ideas what might be causing this?

[Kobzol]

Probably some non-deterministic data structure iteration or insertion somewhere, related to unicase. I'll take a look tomorrow.

[Kobzol]

#111 should hopefully fix this. Let me know if you still run into non-determinism even after this PR gets merged.

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

looks like the non-determinism was also the cause of this

Rebased and regenerated

[Kobzol]

Yeah, I think so. The problem is that if we have two names or e-mails with different casing, it's not really possible to determine which one of those is the "canonical" one (unless they are stored in the mailmap). So we must have deterministic iteration order to always pick the same one, out of multiple options, in order not to have differing output.

[Kobzol]

Thanks! Left a few comments. Where is the all-time.csv file, btw? It should be generated now.

As I mentioned earlier, I would prefer for the tests to live within Rust code, and not the CI config, but I don't want to block this PR on that, because we really need to finally have some tests to be able to move forward with marking changes, and most importantly speeding thanks up.

[DanielEScherzer]

Thanks! Left a few comments. Where is the all-time.csv file, btw? It should be generated now.

Incorporated the feedback and added all-time data

As I mentioned earlier, I would prefer for the tests to live within Rust code, and not the CI config, but I don't want to block this PR on that, because we really need to finally have some tests to be able to move forward with marking changes, and most importantly speeding thanks up.

Yeah, hopefully once things are sped up doing things via a command in the test won't slow things down too much, that is something I plan to work on

[Kobzol]

Thanks! I'm currently experimenting with using a completely different approach for loading the commits, which could speed it up a lot.

[Kobzol]

Hmm, the all time results do not match, for some reason.