Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend Test Module Documentation & Explain Python integration #5333

Merged

Conversation

josegomezr
Copy link
Contributor

  • Extended the OpenQA Test Module documentation with improved structure and examples.

    The module interface is documented (all the possible routines and their purpose are explained & trackable in the sidebar).

  • Explained how the python support works in openQA.

    Added a small section on how python support is provided by openqa, as well as the limitations that the FFI Layer brings.

  • Reorganized the example test modules.

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching docs/*.asciidoc:

  • Consider generating documentation locally to verify it is rendered correctly using tools/generate-htmldoc

This is an automatically generated QA checklist based on modified files.

Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bit of over-capitalization in the git commit message subject line :)

  • Also s/openqa/openQA/
  • Please explain "FFI Layer"

Only checked the git commit message. Would need to check the rest at some later point in time

@josegomezr josegomezr force-pushed the extend_test_module_doc_n_python branch 2 times, most recently from 1cf6311 to bd1a2c6 Compare October 17, 2023 13:02
@josegomezr
Copy link
Contributor Author

Please explain "FFI Layer"

Inline::Python is a wrapper of the python API providing perl capabilities to eval python snippets. It wraps the CPython C API.

It's then used in os-autoinst/autotest.pm circa line 120 ther's a self evaled string block that imports Inline::Python:

#[...]

use Inline::Python qw(py_eval py_bind_func py_study_package);
py_eval(<<'END_OF_PYTHON_CODE');
#[...]
END_OF_PYTHON_CODE

With py_eval the provided python test module is injected into the perl context. The py_study_package and py_bind_func are there to couple the python context with the perl context so they can call each other out.

Plus some sys.path manipulation to make everything available "like magic".

The important part is:

  1. This is not a python binary invocation. All of this python code is being executed by Perl directly with the Python C API.
  2. The Python version used will depend on which python devel headers are available to Inline::Python on installation time.
  3. The python code has an amount of context switches between python and perl, but I don't think they are that dramatic tbh.
  4. I can't confirm it 100% yet (it's been quite complex to get this working on my local), but this python binding is independent from the system's python binary since it's going through the CPython C API. This could be problematic if these sections of the code are running in an environment with multiple python versions, and there it can be confusing to know what python version is actually running.

docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
-------------------------------------------------------------------

=== Example Perl Test Modules
[id="testmodule_perl_examples"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is better to insert IDs before (and not after) their headings so when following a reference the heading itself is shown.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I switched the lines for the [id= and the === and didn't affect at all the id generation. it marks the h3 with that ID regardless of the position.

Also the headers above do it like that too.

What do you mean by "following a reference"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the headers above do it like that too.

We're trying to improve the documentation for years but it isn't perfect yet.

What do you mean by "following a reference"?

Clicking on an internal cross reference within the document or following a URL with hash like https://open.qa/docs/#snapshots. Just have a look at this URL. When loading this page you'll end up below the actual heading. It would be much nicer if the heading was still visible which is achieved by moving the ID before the heading. Of course one could also just use the auto-generated ID (and only that is the ID that marks the h3 element, see e.g. https://open.qa/docs/#_using_snapshots_to_speed_up_development_of_tests).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, I see now.

It looks like a deeper problem with the CSS Layout, normally the referred id should be the first item in the viewport. I don't quite know rn how to improve it without going way off-track with CSS changes or adding workarounds in the asciidocs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally the referred id should be the first item in the viewport.

It should be but that's also not the problem here. If you use the [id="…"] you'll get the ID on the next paragraph. So the paragraph ends up as the first item in the viewport and not the heading. Maybe the HTML generator behaves not like that in all cases (e.g. when the element after the heading is no paragraph but something else) but it makes still generally sense to put the ID before the heading.

Copy link
Contributor

@Martchus Martchus Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if you wanted to change the ID of the heading element itself then you would write it like this:

[[usersguide]]
= openQA users guide

That would likely be the cleanest solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be but that's also not the problem here. If you use the [id="…"] you'll get the ID on the next paragraph.

Just checked this, and it doesn't render like that. IIUC this:

=== Example Perl Test Modules
[id="testmodule_perl_examples"]

is expected to render:

<!-- [...] -->
<h3>Example Perl test modules</h3>
<!-- the next element in line has the ID -->
<div id="_example_perl_test_modules"> class="paragraph">
<!-- [...] -->

But it actually renders:

<!-- [...] -->
<h3 id="_example_perl_test_modules">Example Perl test modules</h3>
<div id="testmodule_perl_examples" class="paragraph">
<!-- [...] -->

HTML seems to be rendered correctly with [id=...] below the heading.

docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
implementing the interface described above.

==== OpenQA Web UI Sample test
[id="testmodule_python_webui"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these IDs even used at all? If not then it would be best to just delete them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They can be handy for bookmarking/hotlinking but no idea if there used or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to have something direct to bookmark

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not required for bookmarking as you'll get an auto-generated ID in any case (e.g. https://open.qa/docs/#_sending_new_lines_and_continuation_characters).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not required for bookmarking as you'll get an auto-generated ID in any case (e.g. https://open.qa/docs/#_sending_new_lines_and_continuation_characters).

i'd still prefer something intentional over autogenerated but it isn't the end of the world

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do get used, but there's no agreement apparently.

see a sample of files having #open.qa/docs#

assets/javascripts/running.js
container/helm/README.md
docs/UsersGuide.asciidoc
lib/OpenQA/CLI/api.pm
lib/OpenQA/Schema/Result/Jobs.pm
lib/OpenQA/WebAPI/Plugin/Helpers.pm

I don't have a strong opinion on keeping or removing. Adding them keeps the ID's stable (and in turn the bookmark) even if the title changes. Removing it means the links will vary with the title.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, if it is very important that the ID stays stable then you should keep the explicit ID. I guess that's what they're for. But then still place it before the heading.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do get used, but there's no agreement apparently.

see a sample of files having #open.qa/docs#

assets/javascripts/running.js
container/helm/README.md
docs/UsersGuide.asciidoc
lib/OpenQA/CLI/api.pm
lib/OpenQA/Schema/Result/Jobs.pm
lib/OpenQA/WebAPI/Plugin/Helpers.pm

I don't have a strong opinion on keeping or removing. Adding them keeps the ID's stable (and in turn the bookmark) even if the title changes. Removing it means the links will vary with the title.

lets leave the follow ups for when we have a technical writer.

@okurz
Copy link
Member

okurz commented Oct 17, 2023

Please explain "FFI Layer"

Inline::Python is a wrapper of the python API providing perl capabilities to eval python snippets. It wraps the CPython C API.

Sorry, I meant make your code and git commit messages explain that, not explain to me on github :)

@josegomezr
Copy link
Contributor Author

Sorry, I meant make your code and git commit messages explain that, not explain to me on github :)

My bad 😅 but hope that the latest changes (thanks to @Martchus & @kalikiana) explain better the integration.

@josegomezr josegomezr force-pushed the extend_test_module_doc_n_python branch from 2841fcb to ebe9a6f Compare October 17, 2023 19:14
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
@josegomezr josegomezr force-pushed the extend_test_module_doc_n_python branch from 8cd7680 to 10fe121 Compare October 19, 2023 13:50
@Martchus
Copy link
Contributor

Should we squash the commit messages? Not all would satisfy our normal criteria and I guess in this case not much is lost from squashing.

- Extended the openQA Test Module documentation with improved
  structure and examples.

  The module interface is documented (all the possible routines and
  their purpose are explained & trackable in the sidebar).

- Explained how the python support works in openQA.

  Added a small section on how python support is provided by
  openQA, as well as the limitations that 'Inline::Perl' brings.

- Reorganized the example test modules.

- Perl-tidy'd perl examples.

Co-authored-by: Liv Dywan <liv@twotoasts.de>
Co-authored-by: Martchus <martchus@gmx.net>
Co-authored-by: Oliver Kurz <okurz@suse.de>
@josegomezr josegomezr force-pushed the extend_test_module_doc_n_python branch from 10fe121 to 224b709 Compare October 19, 2023 15:17
@okurz
Copy link
Member

okurz commented Oct 19, 2023

Should we squash the commit messages? Not all would satisfy our normal criteria and I guess in this case not much is lost from squashing.

Yes, thank you @Martchus . Fixed meanwhile so I guess you can approve now.

docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
docs/WritingTests.asciidoc Outdated Show resolved Hide resolved
Co-authored-by: Tina Müller (tinita) <cpan2@tinita.de>
@mergify mergify bot merged commit 83ab402 into os-autoinst:master Oct 20, 2023
36 checks passed
os-autoinst-bot pushed a commit to os-autoinst-bot/openQA that referenced this pull request Oct 21, 2023
commit 83ab402
Merge: d58c4ff ed91efc
Author:     mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
AuthorDate: Fri Oct 20 08:28:04 2023 +0000
Commit:     GitHub <noreply@github.com>
CommitDate: Fri Oct 20 08:28:04 2023 +0000

    Merge pull request os-autoinst#5333 from josegomezr/extend_test_module_doc_n_python

    Extend Test Module Documentation & Explain Python integration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants