Merge ~cjwatson/launchpad:ci-build-multi-arch-ui into launchpad:master

[Colin Watson]

Clarify CI build links when they exist for multiple architectures

If you set up a `.launchpad.yaml` that runs builds on multiple architectures, the UI presentation currently looks like this:

  [ ] build:0 (build)
  [ ] build:0 (build)

That leaves something to be desired in terms of clarity. Show the series and architecture in the link text that currently just reads "build", so that it looks like this instead:

  [ ] build:0 (focal/amd64)
  [ ] build:0 (focal/arm64)

[Jürgen Gmach (jugmac00)]

Great improvement!

Would it be possible to make the tests as expressive as the commit message?

I would love to see that we assert against literals more often.

This makes the tests easier to read and to debug, and in case a test breaks it is much easier to see at one glance what is going wrong.

review: Approve

[Colin Watson (cjwatson)]

I think we've previously established that we disagree about literals - I've seen too many cases where assertions on literals masked problems where things were inappropriately hardcoded in the code, but hey, they worked for that particular literal value! That's why I always prefer to use the object factory where I can and let it autogenerate test strings for me.

Perhaps we can find a compromise though. Could you point to exactly which bit of the tests you don't feel are expressive enough?

[Jürgen Gmach (jugmac00)]

Thanks for keeping this discussion going. Having tests "trivially correct upon inspection" is one of several building blocks which I would love to see for Launchpad to be incorporated, to make maintaining such a huge project, which was written and is and will be maintained by many generations of developers over decades more sustainable.

I see your point. I have been bitten by hardcoded test values myself. On the other hand, I have seen tests where both the input and the output were generated, and although both were wrong, the tests passed. You'd need to step through the tests in order to prove they do what they claim.

The issue with a hardcoded value can be mitigated by testing several values. In this special case we could use a couple of series and architectures. This is trivial with pytest's parametrization feature, but should also be also possible with our setup, either with subtests https://blog.ganssle.io/articles/2020/04/subtests-in-python.html or with asserting in a loop.

In case of "using several input values to mitigate the fact of one hardcoded value could mislead us" is not enough of a compromise, I also thought about adding comments.

e.g.
```
text=re.compile(r".*/.*"),
```
turns into
```
text=re.compile(r".*/.*"), # e.g. "focal/arm64"
```

This helps with understanding the test, but unfortunately does not help with "trivially correct upon inspection". And it brings all the problems along of diverging code and comments.

And as you asked me about where exactly to improve expressiveness - right after the "text=..." - there should be a string, e.g. `text=focal/arm64`, just as above in your commit message.

I have incorporated this kind of writing tests many years ago, and I strongly believe this is the way to go. As you mentioned, we had the discussion before and we agree to disagree. I respect that, and that is why I have approved the MP.

We should also keep two things in mind. Whenever possible we should write code in a way that it can be easily understood by all team members, not just by oneself. And as senior engineers, it is not just about a personal preference, every decision has a bigger impact.

By the way, the quote from above, i.e "trivially correct upon inspection", is from a book I just recently started reading: "Software Engineering at Google" - in which a couple of google engineers discuss what they have learned from maintaining massive code bases for decades. It is not like we need take everything written as gospel truth, but I have to admit it is kinda nice to provide a link to a highly regarded book to support one's standpoint:
https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests

Unmerged commits

fc18aef... by Colin Watson on 2023-08-23

Clarify CI build links when they exist for multiple architectures

If you set up a `.launchpad.yaml` that runs builds on multiple
architectures, the UI presentation currently looks like this:

  [ ] build:0 (build)
  [ ] build:0 (build)

That leaves something to be desired in terms of clarity. Show the
series and architecture in the link text that currently just reads
"build", so that it looks like this instead:

  [ ] build:0 (focal/amd64)
  [ ] build:0 (focal/arm64)

	docs:0 (build)
	lint:0 (build)
	mypy:0 (build)

1 → 3 of 3 results

First • Previous • Next • Last

diff --git a/lib/lp/code/browser/tests/test_gitref.py b/lib/lp/code/browser/tests/test_gitref.py
index 0c02bac..8b5e8d3 100644
--- a/lib/lp/code/browser/tests/test_gitref.py
+++ b/lib/lp/code/browser/tests/test_gitref.py
@@ -277,7 +277,9 @@ class TestGitRefView(BrowserTestCase):
                     soupmatchers.Within(
                         soupmatchers.Tag("first report title", "td"),
                         soupmatchers.Tag(
-                            "first report CI build link", "a", text="build"
+                            "first report CI build link",
+                            "a",
+                            text=re.compile(r".*/.*"),
                         ),
                     )
                 ),
@@ -307,7 +309,11 @@ class TestGitRefView(BrowserTestCase):
                     soupmatchers.Tag(
                         "second report CI build link",
                         "a",
-                        text="build",
+                        text="%s/%s"
+                        % (
+                            report2.ci_build.distro_series.name,
+                            report2.ci_build.arch_tag,
+                        ),
                         attrs={
                             "href": canonical_url(
                                 report2.ci_build, force_local_path=True
diff --git a/lib/lp/code/templates/git-macros.pt b/lib/lp/code/templates/git-macros.pt
index 3960408..254a805 100644
--- a/lib/lp/code/templates/git-macros.pt
+++ b/lib/lp/code/templates/git-macros.pt
@@ -238,7 +238,10 @@
               <td tal:define="url python: report.url if report.url is not None else path('report/latest_log/download_url | nothing')">
                 <a tal:attributes="href url" tal:content="report/title" />
                 <tal:ci_build condition="report/ci_build">
-                  (<a tal:attributes="href report/ci_build/fmt:url">build</a>)
+                  (<a tal:define="archseries python: report.distro_arch_series or report.ci_build.distro_arch_series"
+                      tal:attributes="href report/ci_build/fmt:url"
+                      tal:content="string:${archseries/distroseries/name}/${archseries/architecturetag}"
+                      />)
                 </tal:ci_build>
               </td>
               <td tal:content="report/result_summary" />