Tests: fixup: explicitly unset SOURCE_DATE_EPOCH during 'test_html_multi_line_copyright' #13224
Conversation
…t_html_multi_line_copyright`
|
cc also @cjwatson; I'll admit that I had scanread the Debian bug yesterday, albeit without looking at your patch in any detail; I wrote this suggestion separately today after investigating and understanding the bug, and.. it's near-identical. I'm happy to defer to yours instead if you'd prefer (you wrote it first, and strictly speaking I had read the bug thread). |
|
(NB: I only compared the content of the patches after developing and testing this one.. difficult to prove that, though, I suppose) |
|
I have no sense of proprietary ownership here, and the fix seemed fairly obvious after understanding the problem. Go ahead. |
|
Thank you. Despite that, I'd recommend some caution from the maintainers before/if merging this; I think people without context could reasonably be skeptical that I authored it, and perhaps even if I think I did, I may have subconsciously picked up the details yesterday since it's a small patch. |
|
It doesn't worry me, but if you want to, you could include something like |
|
Possibly; in this context I'm currently skeptical about whether co-authorship is an accurate assessment. I think you authored the patch first, and then I apparently authored an equivalent patch independently (but as noted, I treat myself as an unreliable narrator). |
|
I really don't mind, and while I appreciate your diligence, with all due respect I think you're probably overthinking this. This patch is straightforward enough, and the new fixture is clearly similar enough to the existing |
|
Also, the main reason I hadn't submitted it myself is that I hadn't got round to writing up my reasoning in a form suitable for a PR description. You did that in a way that's probably clearer than I would have done. |
| @pytest.fixture | ||
| def source_date_empty(monkeypatch): | ||
| with monkeypatch.context() as m: | ||
| m.delenv('SOURCE_DATE_EPOCH', raising=False) | ||
| yield |
There was a problem hiding this comment.
If we only use this once, is it worth being a standalone function?
If we should keep it, could we add a docstring to describe why and when we should use the fixture?
A
There was a problem hiding this comment.
When I experimented with this, I initially tried inlining it into the relevant test, but that didn't work since the monkeypatch needs to be applied before the app fixture is started.
Purpose
Sphinx's copyright substitution currently allows years identified as the current year to be downgraded to previous years when
SOURCE_DATE_EPOCHis configured, to assist reproducibility of documentation builds.However, we have a test case
test_html_multi_line_copyright, written in Y2024, that mentioned a future year - Y2025.Now that that year is current, it is eligible for substitution when
SOURCE_DATE_EPOCHis configured. Many buildsystems, such as those used by Debian and Fedora, choose the most-recent packaging/commit timestamp to use asSOURCE_DATE_EPOCH's timestamp, since those corresponds sensibly to a time-of-build.However, for the v8.1.3 Sphinx release including the updated substitution logic, the year-of-release/commit was Y2024. Thus, if a commit/packaging date from that year is chosen, and
SOURCE_DATE_EPOCHis configured when the unit tests run, thetest_html_multi_line_copyrighttest will fail.The fix suggested here is to explicitly unset
SOURCE_DATE_EPOCHwithintest_html_multi_line_copyright.References
Edit: add a reference to the pull request that introduced the bug