Repository navigation
GitHub cli tests incompatible with latest Glamour v0.8.0 #10179
Description
Description
Latest version of https://github.com/charmbracelet/glamour, which GitHub cli depends on, has slightly different behavior in whitespace and line width, which makes tests fail. The correct fix is to simply modify the tests slightly. There is no actual bug, end result for GitHub cli users is still correct before and after.
Steps to reproduce the behavior
- Update build dependency Glamour to v0.8.0
- Build
Expected vs actual behavior
Build and post-build test suite should pass.
However, with Glamour v0.8.0 they start failing. One example below.
Logs
=== RUN TestPRReview_interactive
review_test.go:283:
Error Trace: /<<PKGBUILDDIR>>/_build/src/github.com/cli/cli/v2/pkg/cmd/pr/review/review_test.go:283
Error: Not equal:
expected: "Got:\n\n cool story \n\n"
actual : "Got:\n\n cool story \n\n"
Diff:
--- Expected
+++ Actual
@@ -2,3 +2,3 @@
- cool story
+ cool story
Test: TestPRReview_interactive
--- FAIL: TestPRReview_interactive (0.00s)
Solution
Merge #10151.
Additional details
The original error was reported in Debian in https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1091585 and fixed in https://salsa.debian.org/go-team/packages/gh/-/commit/8faccf149d0836f8ff285808182c15cf02dd9f28 that was included in the latest upload the Debian unstable.
Activity
Update test to be compatible with latest Glamour v0.8.0
williammartin commentedon Jan 6, 2025
Thanks for creating this @ottok.
Did you really mean GitLab here? What confused me about the original PR (and forgive my ignorance of debian practices here) is why
ghwas being build with a later version of glamour rather than the version we specify. This would be useful context for us to keep in mind going forward.Is there some policy to build with the latest minor versions? https://salsa.debian.org/go-team/packages/gh/-/blob/8faccf149d0836f8ff285808182c15cf02dd9f28/debian/control#L15
Cheers!
jtmcg commentedon Jan 6, 2025
Thanks for opening this issue, @ottok. I appreciate you taking the time 🙌
@williammartin's question is mine as well 🙂 I want to understand why debian needs us to update glamour so we can avoid issues like this in the future for y'all
ottok commentedon Jan 7, 2025
Sorry for the name typo, fixed now to GitHub.
Debian has only one version of any Go package at any given time. The version history for Glamour can be viewed at https://tracker.debian.org/pkg/golang-github-charmbracelet-glamour. When it updated from 0.6.0 to 0.8.0 all Go packages in Debian that use Glamour started using the latest version (next time they get built).
This is not a problem. You will eventually upgrade from 0.7.0 to 0.8.0 or 0.9.0? If not in 2025, then surely in 2026 or 2027. Whenever you are ready to take the plunge you can merge #10151 instead of doing that work yourself.
williammartin commentedon Jan 8, 2025
Thanks. What happens when there was a change in a dependency that we would not upgrade to, say because it had a meaningful behaviour change (more than just whitespace)? Would you allow
ghin the debian repository to diverge from us in behaviour?I don't see an issue with this change specifically, and it seems reasonable to merge your PR sooner, rather than later, I'm just trying to get my head around the expectations.
penguin359 commentedon Jan 13, 2025
I would recommend adding a step to normalize the whitespace in command output as opposed to just adjusting the expected values with the newer dependency upgrades. Ideally, the test shouldn't care whether the output ended in 8 spaces or 10 spaces and transform to just collapse the whitespace down to a single space (at least for ASCII space, if not newline and carriage return), and then the same test will work on both pre 0.8 and post 0.8 versions of Glamour. I might try to take a whack at this later if I get enough free time.
ottok commentedon Jan 13, 2025
To me it seems that the tests specifically were crafted to match against the exact output of Glamour. Hence if you collapse the whitespace the tests become partially moot.
williammartin commentedon Jan 22, 2025
@ottok nudge on:
Thanks.
Update test to be compatible with latest Glamour v0.8.0
Update test to be compatible with latest Glamour v0.8.0
ottok commentedon Jan 23, 2025
Depends on the overall situation. The goal for the Linux distribution is to have one version of each program. Occasionally new and old versions of programs are kept around if the new/old is significantly different and there is demand for both to exist. For example I am currently working to have Godot 3.6 and 4.3 in Debian at the same time (with the version appended to the name) because a lot of games only work with old Godot 3.6 while latest games require latest Godot 4.3.
ottok commentedon Jan 23, 2025
And yes, Linux distributions constantly add patches on top of upstream code to change behaviour. It is up to the distro maintainers to decide when/what delta to upstream make sense and for how long.
GitHub cli patches in Debian and their upstream status is tracked at https://udd.debian.org/patches.cgi?src=gh&version=2.46.0-3
Source code at https://salsa.debian.org/go-team/packages/gh/-/tree/debian/sid/debian/patches
williammartin commentedon Jan 23, 2025
Great, thanks for the info.