Hi,

Thanks for looping back to this.

On Sat, 2022-04-16 at 13:28 +0200, Marius Kriegerowski wrote:

After some time I found the time and took a deeper dive into patchtest. It’s not
in the prettiest state and has a couple of design choices which make me wonder
if it wouldn’t be better to go for a complete rewrite. e.g. I would rather opt
for pytest as it’s much more lightweight and easier to work with and more modern
than unittest. But let me dive deeper first.

I've wondered if we do need to re-invent it but for different reasons. It
currently integrates with patchwork but for example it may be better to have it
work off lore and it may be better using some of the more modern patchwork
tooling. One example I was pointed at was:

https://git.kernel.org/pub/scm/linux/kernel/git/mricon/korg-helpers.git/tree/git-patchwork-bot.py

With regard to unitest vs pytest, please do use unittest. The reason I say that
is the rest of the project's testing runs within unitest so I'd prefer to have
one standard rather than two. You can see it here:

https://git.yoctoproject.org/poky/tree/meta/lib/oeqa

(there are selftests, runtime tests, sdk tests, eSDK tests, performance tests
and so on).

Patchtest is hardly documented which makes it hard to follow. Or is there any
more documentation out there outside of the repo? I would start writing
documentation but want to make sure that I don’t repeat what others have done
before.

Sadly there isn't :(.

There are selftests but they don’t work:
https://github.com/HerrMuellerluedenscheid/patchtest/tree/overhaul/selftest
Can these be removed?

That directory appears to contain a load of training/test data. I don't care so
much about the actual test mechanism but surely the old training data is a good
place to start to test any new instance that is brought up?

I’m assuming that in the CI context the `--json` flag is used to produce json
output which is then somehow parsed and returned to the user, right? I would
like to work with the DevOps integration team to make sure I’m not deviating too
far from what the integration team expects.

It looks like you're working on "patchtest-oe" from
https://git.yoctoproject.org/patchtest-oe/ . There is a second piece to the
puzzle, a harder piece unfortunately which is
https://git.yoctoproject.org/patchtest/ .

The latter is the piece which is effectively the glue logic between patchwork
and patchtest-oe.

I'm not sure what you're aiming for as an end result. To recap, what used to
happen is that patchwork+patchtest would see new patches on the mailing list,
pick them up, run the tests in patchtest-oe against them, then reply on the
mailing list if there were any issues detected. We would like to get back to
this.

Unfortunately we were running a forked/hacked up version of patchwork which we
couldn't update. We ended up switching to a vanilla upstream version of it so it
is now here: https://patchwork.yoctoproject.org/ however that broke patchtest.

What we'd like to do is:

a) Get patchtest back up and running against new patches on the mailing list in
some form. As I hinted at above, this may mean using a different approach to
what patchtest used to do.

b) Move the actual tests in patchtest-oe to meta/lib/oeqa/ in OE-Core as a new
set of tests.

c) Add a script in OE-Core so that you can run the tests in patchtest-oe (now in
OE-Core) against a patch before you send it to the mailing list

d) Anyone else could then reuse these tests as part of CI, e.g. on
github/gitlab/whatever

I managed to make it run on my machine but even if one test fails, it returns
OK in the end. Was that implemented on purpose to make this work in the CI
context?

I suspect patchtest would have code to read and handle that.

So, just for me to know if I’m on the right track, does this look like a
reasonable output indicating that it works fine?

❯ patchtest ../poky/0001-scriptutils-fix-style-to-be-more-PEP8-compliant.patch
../poky ../patchtest-oe/tests
Testing patch ../poky/0001-scriptutils-fix-style-to-be-more-PEP8-
compliant.patch
run pre-merge tests
BUILDDIR is not defined. Cannot load bitbake
SKIP setUpClass (test_metadata_lic_files_chksum.LicFilesChkSum)
BUILDDIR is not defined. Cannot load bitbake
SKIP setUpClass (test_metadata_src_uri.SrcUri)
SKIP test_python_pylint.PyLint.pretest_pylint
----------------------------------------------------------------------
Ran 1 test in 0.548s

OK
run post-merge tests
PASS test_mbox_author.Author.test_author_valid
PASS test_mbox_author.Author.test_non_auh_upgrade
PASS test_mbox_bugzilla.Bugzilla.test_bugzilla_entry_format
SKIP test_mbox_description.CommitMessage.test_commit_message_presence
PASS test_mbox_format.MboxFormat.test_mbox_format
PASS test_mbox_mailinglist.MailingList.test_target_mailing_list
FAIL test_mbox_merge.Merge.test_series_merge_on_head
PASS test_mbox_shortlog.Shortlog.test_shortlog_format
PASS test_mbox_shortlog.Shortlog.test_shortlog_length
FAIL test_mbox_signed_off_by.SignedOffBy.test_signed_off_by_presence
BUILDDIR is not defined. Cannot load bitbake
SKIP setUpClass (test_metadata_lic_files_chksum.LicFilesChkSum)
BUILDDIR is not defined. Cannot load bitbake
SKIP setUpClass (test_metadata_license.License)
SKIP test_metadata_max_length.MaxLength.test_max_line_length
BUILDDIR is not defined. Cannot load bitbake
SKIP setUpClass (test_metadata_src_uri.SrcUri)
BUILDDIR is not defined. Cannot load bitbake
SKIP setUpClass (test_metadata_summary.Summary)
SKIP test_patch_cve.CVE.test_cve_tag_format
SKIP test_patch_signed_off_by.PatchSignedOffBy.test_signed_off_by_presence
SKIP
test_patch_upstream_status.PatchUpstreamStatus.test_upstream_status_presence_f
ormat
SKIP test_python_pylint.PyLint.test_pylint
----------------------------------------------------------------------
Ran 15 tests in 0.260s

OK

I believe that patchtest had some minimal bitbake environment which would have
allowed some of the skipped tests to run. I'm not an expert on this and I don't
remember how the handoff happened with it though.

Hope this offers at least some guideance on where things were and where we need
to be, at least in my view! :)

Cheers,

Richard