Attempt to make tests on CI more robust
ClosedPublic

Authored by dvratil on Nov 22 2019, 10:56 AM.

Details

Summary

akonadi_control: announce new Resource once both services are registered

Make sure that we don't emit agentInstanceAdded until the new Resource has
registered both it's Agent service name and Resource service name.

Testrunner: wait until all jobs are done before aborting setup

When resource setup fails for any reason, we must wait for the remaining
jobs to finish before shutting down Akonadi and terminating the testrunner.
Otherwise the next scheduled job gets executed and causes a new Akonadi
instance to be launched through DBus activation. This instance is not
managed by the testrunner though, so it does not ever get terminated,
causing ctest to hang.

Diff Detail

Repository
R165 Akonadi
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dvratil created this revision.Nov 22 2019, 10:56 AM
Restricted Application added a project: KDE PIM. · View Herald TranscriptNov 22 2019, 10:56 AM
Restricted Application added a subscriber: kde-pim. · View Herald Transcript
dvratil requested review of this revision.Nov 22 2019, 10:56 AM
dfaure added inline comments.Nov 22 2019, 11:09 AM
autotests/libs/testrunner/setup.cpp
98 ↗(On Diff #70147)

here you call only setupFailed....

123 ↗(On Diff #70147)

... while here you call setupFailed followed by checkSetupDone.

How about always calling both (on error) so that you can remove the if+shutdown in setupFailed?
And then setupFailed can go away and you can just do mExitCode = 1; in both places where you call setupFailed currently.

IOW this means turning lines 99-100 into } else { so that checkSetupDone() is called in all cases.

src/akonadicontrol/agentmanager.cpp
620

(too many spaces in the string constants here)

671

I admit I don't fully understand the logic here.

dvratil marked 3 inline comments as done.Nov 22 2019, 1:27 PM
dvratil added inline comments.
src/akonadicontrol/agentmanager.cpp
671

We want to detect when the resource was running, crashed and was started again - in that case we will already have its DBus interface cached (hasResourceInterface == true) and we don't want to announce such event as agentInstanceAdded.

dvratil updated this revision to Diff 70157.Nov 22 2019, 1:27 PM
  • Address review comments
dvratil updated this revision to Diff 70158.Nov 22 2019, 1:30 PM

Fix the patch, arc sucks.

Fix the patch, arc sucks.

Off topic, but @mlaurent told me to use arcanist because my commandline generated 'git diff' patches suck. what's the deal?

Fix the patch, arc sucks.

Off topic, but @mlaurent told me to use arcanist because my commandline generated 'git diff' patches suck. what's the deal?

It handles multi-commit changes incorrectly or not at all. Which is known about arcanist, but I'm just so used to Github/Gitlub at this point, this makes me grumpy...Phabricator is a decent review tool, but arcanist IMO sucks, compared to how easy Github/Gitlab flow is...

dfaure accepted this revision.Nov 22 2019, 2:10 PM

arc diff HEAD~ is my workaround for multi-commit branches.

This revision is now accepted and ready to land.Nov 22 2019, 2:10 PM
This revision was automatically updated to reflect the committed changes.

Fix the patch, arc sucks.

Off topic, but @mlaurent told me to use arcanist because my commandline generated 'git diff' patches suck. what's the deal?

It handles multi-commit changes incorrectly or not at all. Which is known about arcanist, but I'm just so used to Github/Gitlub at this point, this makes me grumpy...Phabricator is a decent review tool, but arcanist IMO sucks, compared to how easy Github/Gitlab flow is...

<offtopic>I am eager to move to gitlab as well. </offtopic>