[Development] Solutions for ensuring that changes in upstream modules are tested with downstream modules before merging

[Mitch Curtis]

> -----Original Message-----
> From: Volker Hilsheimer <volker.hilsheimer at qt.io>
> Sent: Friday, 4 June 2021 4:51 PM
> To: Mitch Curtis <mitch.curtis at qt.io>
> Cc: development at qt-project.org
> Subject: Re: [Development] Solutions for ensuring that changes in upstream
> modules are tested with downstream modules before merging
> 
> > On 4 Jun 2021, at 16:10, Mitch Curtis <mitch.curtis at qt.io> wrote:
> >
> >> -----Original Message-----
> >> From: Volker Hilsheimer <volker.hilsheimer at qt.io>
> >> Sent: Friday, 4 June 2021 2:45 PM
> >> To: Mitch Curtis <mitch.curtis at qt.io>
> >> Cc: development at qt-project.org
> >> Subject: Re: [Development] Solutions for ensuring that changes in
> >> upstream modules are tested with downstream modules before merging
> >>
> >>> On 4 Jun 2021, at 13:56, Mitch Curtis <mitch.curtis at qt.io> wrote:
> >>>
> >>> Hi.
> >>>
> >>> A common problem I see is that a change in say qtbase or
> >>> qtdeclarative can
> >> cause test failures in modules that depend on them after that change
> >> is merged. As a result, dependency updates for the downstream modules
> >> can be blocked, requiring the module maintainer to look into the
> >> failure, only to discover that it is caused by an upstream module.
> >> This causes a lot of time and effort to be diverted away from regular
> >> work. Both upstream and downstream developers would benefit from
> >> having more immediate CI feedback on these kinds of changes.
> >>>
> >>> https://bugreports.qt.io/browse/QTBUG-79454 was created to track
> >>> this
> >> problem. So far the ideas suggested have been:
> >>>
> >>> - Run tests of dependent modules when testing a change, in addition
> >>> to
> >> the regular tests for that module
> >>> - Merge repositories of more tightly coupled modules (e.g. move
> >> qtquickcontrols2 into qtdeclarative)
> >>>
> >>> It would be beneficial to discuss the advantages and disadvantages
> >>> of
> >> these and other solutions so that we can address this problem.
> >>>
> >>> Cheers.
> >>
> >>
> >> Excellent topic for the upcoming Qt Contributors Summit, Mitch, great
> >> if you can add it to the list of topics:
> >>
> >> https://wiki.qt.io/Qt_Contributors_Summit_2021_-_Program
> >>
> >> Which doesn’t mean that we shouldn't start the discussion on the
> >> list, and maybe we can then conclude it at the summit.
> >>
> >>
> >> Cheers,
> >> Volker
> >
> > OK, added it to the list.
> >
> 
> Thanks.
> 
> 
> As for the topic:
> 
> I do think that we have cases where too many modules are lumped into a
> single repo, and cases where we have too much granularity.
> 
> For the former: I see no immediate reason why QtNetwork needs to be in
> qtbase (network tests failing is a major cause of unrelated integrations
> failing), or why the Qt Positioning module needs to be in qtlocation.
> 
> For the latter: Qt Quick Controls 2 might be better off sharing a commit
> history with QtQuick
> 
> However, while we can make adjustments, I also think that there’s no
> perfect solution. It’s always a tradeoff between speed and autonomy on the
> one, and integrity across dependencies on the other hand.

I agree.

> Running more tests is a similar tradeoff, and I don’t think blocking an
> integration into e.g. qtbase because it breaks a leaf module is a good solution
> to the problem. We should aim for fast feedback, and short meantime to
> recover such breakages, and making CI runs take even longer helps with
> neither. In particular if we then can’t make any such changes even though a
> follow up change in a leaf module is already available and just needs to wait
> for the dependency update. Then we are in a deadlock, unless we keep the
> code in qtbase working for both versions of e.g. qtdeclarative, which is not
> always practical.

I could be wrong, but perhaps this is what Ossi's https://bugreports.qt.io/browse/COIN-133 was targeting. In any case, I can't really speak for qtbase as I rarely work on it.

> The problem is perhaps not that we break things, but that we don’t notice
> for too long that we broke things? And once we do notice, we often seem to
> be unwilling to revert the change that did cause the breakage, if things don’t
> get fixed quickly.

This might be the case for qtbase and its leaf modules, but not for qtdeclarative and qtquickcontrols2. The typical situation there is: qtquickcontrols2 dependency update fails. I immediately cannot think of any relevant changes that would cause such a failure, so I quickly look through git log. If I can't find anything there, I need to start bisecting with qtdeclarative (and this assumes that the bisect doesn't result in build errors). It's not a productive use of time.

On the other hand, a CI run, while taking extra time if having to run the tests of two or more modules, shouldn't block someone from doing work. There is always another task to work on while waiting for CI. Flakiness will of course make the feedback loop take way longer, but that has always been the case.

> To know about breakage quicker, a nighly run of all tests against “HEAD of
> everything” could help. Our dependency updates give us some of that, and
> we usually respond swiftly when an update fails. It shouldn’t be on the leaf
> module maintainer alone to follow up on things, and I do have the
> impression that people do take ownership, at least once they see that their
> base module change broke stuff. I’m sure we can improve there as well.
> But now that Qt 6 is out, I’d expect that a “everything at HEAD” toplevel build
> should work most of the time. My local worktrees are usually at HEAD of
> everything, and it’s rare that there are build problems at least.

For qtdeclarative and qtquickcontrols2 I'm not sure how this would improve anything. It would say that a test failed, but as you said, that's what the dependency update already tells us. It does sound like a nice tradeoff (of running tests for all modules on every change and not running them at all) for modules where the dependency updates don't give any signs of a breakage, though.
 
> Lastly, I do think that we could do more in encoding assumptions made
> across modules in unit tests. If a change in qtdeclarative breaks an
> assumption made in qqc2, then ideally there’s a unit test in qtdeclarative that
> would fail. We use private APIs a lot across some modules, but we don’t have
> unit tests for them in the base module.

Agreed, and the qtdeclarative team are great in doing this once they're aware of it, but there are too many use cases for them to have to be aware of in order to fully solve the problem. It's also a slow process (in terms of the rate of breakages across modules) in adding tests upstream to catch issues as they occur downstream.

I would really be interested if there are other leaf modules that have this issue. If it's just qtquickcontrols2 then perhaps merging into qtdeclarative would be the easiest solution after all.