Add Test Cases for patching #258

nephros · 2022-01-27T08:22:14Z

This should probably not be squashed on merge so the individual cases keep their histories.

Olf0 · 2022-02-03T00:00:27Z

@b100dian, IMO this is not at all critical WRT time, so take all the time you need to review this comfortably. But I requested your review (rather meant as "suggested to review this to you"), because I feel a review from me will be far less stringent due to far less know-how (and I already looked over this PR without seeing anything I could criticise). If you are absolutely out-of-time in the foreseeable future, please say so and I will do my best and mark this as reviewed, so nephros can proceed to merge this.

b100dian · 2022-02-05T00:55:04Z

Thanks for adding much missed test cases!
@nephros can you add some details how this works? (in the description of the PR would do)
Specifically I don't understand when the patches are applied.

What I tried: I started the app from the CI build and it failed with

$ /usr/bin/sailfish-qml patchmanager-testcase
[W] unknown:3 - file:///usr/share/patchmanager-testcase/qml/patchmanager-testcase.qml:3:1: module "org.SfietKonstantin.patchmanagertests" is not installed 
     import org.SfietKonstantin.patchmanagertests 1.0 
     ^

Then the app from debug deploy from Sailfish IDE -> starts, displays "Case 1 unpatched Case2 unpatched" (as expected by reading patchmanager-testcase.qml

nephros · 2022-02-06T17:45:32Z

Thanks for adding much missed test cases! @nephros can you add some details how this works? (in the description of the PR would do) Specifically I don't understand when the patches are applied.

What I tried: I started the app from the CI build and it failed with
$ /usr/bin/sailfish-qml patchmanager-testcase
[W] unknown:3 - file:///usr/share/patchmanager-testcase/qml/patchmanager-testcase.qml:3:1: module "org.SfietKonstantin.patchmanagertests" is not installed 
     import org.SfietKonstantin.patchmanagertests 1.0 
     ^
Then the app from debug deploy from Sailfish IDE -> starts, displays "Case 1 unpatched Case2 unpatched" (as expected by reading patchmanager-testcase.qml

I can not reproduce that here, with the package installed (not using an IDE).

Having the qmldir file at $LIBDIR/SfietKonstantin/patchmanagertests/qmldir should make that import statement work (even though there is no real QML module/plugin present) AFAIK.

On my device that looks like this:

nemo@PGXperia10:~/git/patchmanager/repo $ QML_IMPORT_TRACE=true qmlscene /usr/share/patchmanager-testcase/qml/patchmanager-testcase.qml  2>&1 | grep ertests
18:33:33.927 unknown:0 unknown QQmlImports(file:///usr/share/patchmanager-testcase/qml/patchmanager-testcase.qml)::addLibraryImport: "org.SfietKonstantin.patchmanagertests" 1.0 as ""
18:33:33.928 unknown:0 unknown QQmlImports(file:///usr/share/patchmanager-testcase/qml/patchmanager-testcase.qml)::importExtension: loaded "/usr/lib/qt5/qml/org/SfietKonstantin/patchmanagertests/qmldir"
18:33:34.277 unknown:0 unknown QQmlImports(file:///usr/share/patchmanager-testcase/qml/patchmanager-testcase.qml)::resolveType: "TestCase1Item" => "" QUrl("file:///usr/lib/qt5/qml/org/SfietKonstantin/patchmanagertests/TestCase1Item.qml") TYPE/URL

Why did I invent org.SfietKonstantin.patchmanagertests?
In order to have something that lives under LIBDIR so the 32/64 things can be tested.

So the test infrastructure consists of:

The QML module under LIBDIR/qt5/qml/..., which is some QML component files and a qmldir file so they can be used as a module
The little app as a QML file and .desktop file under /usr/share/, which imports the above module and thus shows changes to the module's components.
a set of patches which act on those files in various ways

What do the patches do? I'll cop out of this one and refer to "read the source Luke" here. Not all of them have a visible effect on the app, my main goal was to test the patching process itself, not so much changing the test app (I trust my patches work correctly after they apply correctly and in practice never even open the test app).

Olf0 · 2022-02-23T00:35:40Z

Dear @b100dian, this seems to be a bit deadlocked between you both. I am afraid that @nephros will have to regularly update this branch from the master branch (as it happened by commit 67f8b64), until this is merged.

I considered a "best effort review" from me, which will only scratch the surface because this contains too much non-trivial C++ and QML code for me to review properly. But as @nephros is successfully using the test cases to generate meaningful results and I am happy that he does, I trust that this is working. At least I could let this stalled PR progress this way.

OTOH I value your knowledge and practical programming experience very much @b100dian, so I would rather like to see this properly reviewed (i.e., by you, not me). But if you do lack the time in the foreseeable future, just state so and we deviate from the standard procedures: I perform a "best effort review", this is merged and your review is postponed; although I see the risk of this becoming postponed to "never-never day" as a consequence.

All in all this is not altering existing code and not adding code, which is deployed to users, AFAICS: Hence I do not believe anything really bad can happen ("famous last words™").

Olf0 · 2022-02-26T01:46:09Z

P.S.: »I'll cop out of this one and refer to "read the source Luke" here.« made me chuckle. "But I also want nicely written, in-depth documentation for the test cases in our wiki"! While that is true, I rather have test cases without much documentation than no test-cases at all. Even though I am afraid of the day on which I will face the "read the source Luke" part in practice.

More on the serious side:
IMO the prime objective currently is to shove the test-cases into the master branch while ensuring that they are basically working and usable, in order to get them utilised by more people than @nephros alone.
So yes, if there are issues with the test-cases beyond "What do the patches do?" I think we should clarify these and address them first. And it sounds as if @b100dian noticed that one may run into such issues, assuming that "Sailfish IDE" addresses the "SailfishOS SDK / emulator", which presumably many use for development (not me, though).

As @nephros addressed the "import issue", so this should be at least manually replay-able under the SFOS-SDK, I have to admit that I do not really understand the remaining open question "Specifically I don't understand when the patches are applied."
In complete ignorance (i.e., not having tested any of this yet) I would assume, when one activates them in PM GUI. And further assume (with further ignorance) that the testcase app should then display "Case 1 patched Case 2 patched", when started again via its desktop icon or sailfish-qml patchmanager-testcase with $LIBDIR/qt5/qml/org/SfietKonstantin/patchmanagertests/qmldir present.

nephros · 2022-02-26T18:03:54Z

In complete ignorance (i.e., not having tested any of this yet) I would assume, when one activates them in PM GUI. And further assume (with further ignorance) that the testcase app should then display "Case 1 patched Case 2 patched", when started again via its desktop icon or sailfish-qml patchmanager-testcase with $LIBDIR/SfietKonstantin/patchmanagertests/qmldir present.

Exactly, sorry for not making that clear in my reply.

So the answer to "when are patches applied" is "when you apply them" i.e. using the PM GUI as usual, or /usr/sbin/patchmanager -a <patchname>.

I have summed up some of the comments here in c22a1eb and added some instructions how to use and expand the patching test cases.

b100dian

Thank you both - not intending to deadlock this at all:)
The simple explanation for how to test the patches eluded me, I was looking for some nonexistent automated process and I'm actually glad it is as simple as the added Readme points out 😌
For the error I've seen - I don't have problems with building the test package myself.

The problem with the import is just with the output_rpms attached to the checks of this PR and possibly what will come out in say, chum/OBS build. I haven't had that problem when building myself.

nephros · 2022-02-26T22:06:06Z

OK thanks all.

I will squash/rebase a couple of these, and then merge this.

copy of the first test patch. 32-to-64 uses the following style of diff file: --- a/usr/lib/foo +++ b/usr/lib/foo 32-to-64-alt uses the following style of diff file: --- /usr/lib/foo +++ /usr/lib/foo both are equally valid inputs for `patch -p1 -d /`

Olf0 requested a review from b100dian February 2, 2022 23:55

b100dian approved these changes Feb 26, 2022

View reviewed changes

nephros and others added 12 commits February 26, 2022 23:11

make an app, make two test cases

3593744

add build infrastructure

3afc02f

improve test app

798d3eb

New test case: add new files, register one in qmldir

f997012

improve patch descriptions

f9884ae

prepare for case 2-2

4f1e033

New test case: 64-to-32, all mangle candidates

0fbc3ee

move test-cases into subdir of tests/

62069ac

Add chum metadata for test-cases subpackage

4d03b57

add description of patching test cases

e7395ee

README_patching: Clarify, rectify and beautify

cf10353

nephros force-pushed the test-cases branch from 703d456 to cf10353 Compare February 26, 2022 22:11

nephros merged commit 1ee905b into master Feb 26, 2022

nephros deleted the test-cases branch February 26, 2022 22:29

Olf0 mentioned this pull request Mar 3, 2022

spec file: Add Requires: %{name} to subpackage #290

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Test Cases for patching #258

Add Test Cases for patching #258

nephros commented Jan 27, 2022 •

edited

Loading

Olf0 commented Feb 3, 2022 •

edited

Loading

b100dian commented Feb 5, 2022 •

edited

Loading

nephros commented Feb 6, 2022 •

edited by Olf0

Loading

Olf0 commented Feb 23, 2022 •

edited

Loading

Olf0 commented Feb 26, 2022 •

edited

Loading

nephros commented Feb 26, 2022 •

edited

Loading

b100dian left a comment

nephros commented Feb 26, 2022

Add Test Cases for patching #258

Add Test Cases for patching #258

Conversation

nephros commented Jan 27, 2022 • edited Loading

Olf0 commented Feb 3, 2022 • edited Loading

b100dian commented Feb 5, 2022 • edited Loading

nephros commented Feb 6, 2022 • edited by Olf0 Loading

Olf0 commented Feb 23, 2022 • edited Loading

Olf0 commented Feb 26, 2022 • edited Loading

nephros commented Feb 26, 2022 • edited Loading

b100dian left a comment

Choose a reason for hiding this comment

nephros commented Feb 26, 2022

nephros commented Jan 27, 2022 •

edited

Loading

Olf0 commented Feb 3, 2022 •

edited

Loading

b100dian commented Feb 5, 2022 •

edited

Loading

nephros commented Feb 6, 2022 •

edited by Olf0

Loading

Olf0 commented Feb 23, 2022 •

edited

Loading

Olf0 commented Feb 26, 2022 •

edited

Loading

nephros commented Feb 26, 2022 •

edited

Loading