[LTP] [PATCH v4] doc: add tests catalog page

[Andrea Cervesato]

Hi Petr,

On 2/6/25 00:20, Petr Vorel wrote:
> Hi Andrea,
>
> generally LGTM, thanks for working on it.
>
> 1) clickable (referenced) keys
>
> I asked keys (e.g. needs_root) to be referenced to be clickable to the
> relevant API.  This allows reader to click and read relevant info about the key.
We can't do it because tst_test struct parsing does not generate a 
reference for the attributes, but only for the entire struct.
Maybe there's a sphinx configuration somewhere, but I have no idea, we 
should read documentation. This should be done by a followup patch.
> 2) git commit description of referenced tags
>
> Perl code was able to take git description.  E.g. instead of plain "d2f007dbe7e4"
> it got:
> d2f007dbe7e4 ("userns: also map extents in the reverse map to kernel IDs")
>
> I understand you don't want to bother with it, but it was more descriptive.
> I guess using github API or simple fetching web page on git.kernel.org and
> take content of "commit-subject" would work, but it'd be slow and we could hit
> some access limits from the provider. I can have look into this afterwards (I
> don't want to block this issue).
I seen that, but it's really too slow and I'm not sure if it provides a 
big help honestly. It's a "nice to have" but slow feature that can be 
resolved with 1 click more on the git commit hash. Feel free to try 
adding it after this patch has been merged, but my impression was 
negative, due to the fact network was having issues (timeouts, slow 
website, etc).
> 3) CSS fix of some tables
>
> Code on tables with keys and values (e.g. needs_kconfigs) looks ugly, when there
> are more items.  E.g. needs_kconfigs at icmp_rate_limit01, which has:
> CONFIG_VETH
> CONFIG_USER_NS=y
> CONFIG_NET_NS=y
>
> The ugly part is margin-bottom: 24px; from class="line-block".
> Single item line uses <p></p>.
>
> I looked into docs (https://tables-with-sphinx.readthedocs.io/en/latest/),
> but than I realized it's a simple CSS issue. It can be fixed by adding
> into doc/_static/custom.css:
>
> /* remove margin for multiline cells */
> .rst-content table td div.line-block {
>      margin-bottom: 0;
> }
>
> I can send it as a separate patch if you don't want to bother with it.
I can add that, thanks for checking.
>
>> +def _generate_tags_table(tags):
>> +    """
>> +    Generate the tags table from tags hash.
>> +    """
> nit: conf.py is getting slightly big. I need to remember that public functions
> are these which generates stats.html and test_catalog.html. Maybe having
> functions or setup() which references them on the top would help reader to start
> with relevant code.
> ...
If you mean to move small functions on top, that can be done easily. At 
the moment, I only see the syscall table generator function being a bit 
long. The test catalog generator function is already splitting it into 
multiple functions and it's pretty easy to read.
>> +def _generate_setup_table(keys):
>> +    """
>> +    Generate the table with test setup configuration.
>> +    """
>> +    exclude = [
> This list is slightly confusing for a reader. I would expect there will be no
> options in generated docs nor tags. Obviously they are processed separately.
We can add a comment.
>
>> +        'child_needs_reinit',
>> +        'needs_checkpoints',
>> +        'resource_files',
> Maybe add this? e.g. creat07 requires also creat07_child. That might be useful
> for somebody to see that test needs another LTP binary.
>
>> +        'save_restore',
> This is useful, it shows which /proc/sys files are touched.
>
>> +        'forks_child',
>> +        'hugepages',
> I know that these are cryptic:
> .hugepages = {1, TST_NEEDS},
> .hugepages = {TST_NO_HUGEPAGES},
> but trying to filter out info may cause people prefer using 'git grep' sources
> instead of searching in 'ctrl+F' in this nice documentation.
>
>> +        'options',
>> +        'timeout',
>> +        'runtime',
> Cyril already asked for runtime, IMHO important to have.
It has been moved in the timeout text. No need have it in the table as well.
>
>> +        'ulimit',
> Also ulimit is useful.
>
>
>> +        'fname',
>> +        'tags',
>> +        'doc',
>> +    ]
> ...
>> +def generate_test_catalog(_):
>> +    """
>> +    Generate the test catalog from ltp.json metadata file.
>> +    """
>> +    output = '_static/tests.rst'
>> +    metadata_file = '../metadata/ltp.json'
>> +    text = [
>> +        '.. warning::',
>> +        '    The following catalog has been generated using LTP metadata',
>> +        '    which is including only tests using the new :ref:`LTP C API`.',
>> +        '    For this reason, some old tests might be missing from the list.',
> Besides the fact that *all* tests using legacy API are skipped I don't think
> user is interested why they aren't included. I would just note that only tests
> which use new :ref:`LTP C API` are here listed.
>
>> +        ''
>> +    ]
>> +
>> +    metadata = None
>> +    with open(metadata_file, 'r', encoding='utf-8') as data:
>> +        metadata = json.load(data)
>> +
>> +    timeout_def = metadata['defaults']['timeout']
>> +
>> +    for test_name, conf in metadata['tests'].items():
>> +        text.extend([
>> +            f'{test_name}',
>> +            len(test_name) * '-'
>> +        ])
>> +
>> +        # source url location
>> +        test_fname = conf.get('fname', None)
>> +        if test_fname:
>> +            text.extend([
>> +                '',
>> +                f"`source <{ltp_repo_base_url}/{test_fname}>`__",
>> +                ''
>> +            ])
>> +
>> +        # test description
>> +        desc = conf.get('doc', None)
>> +        if desc:
>> +            desc_text = []
>> +            for line in desc:
>> +                if line.startswith("[Description]"):
>> +                    desc_text.append("**Description**")
> I suggested about deleting all [Description] from the code. I see no value in
> it, but it forces reader to scroll more (it prolongs the page).
>
>> +                elif line.startswith("[Algorithm]"):
>> +                    desc_text.append("**Algorithm**")
>> +                else:
>> +                    desc_text.append(line)
> NOTE: because there are more titles in "[..]" (there can be anything, why not
> just replace \[(.*)\] with **\1** ?
That's how it should be done yes. But at the moment we are still having 
[..], so that should be removed with a followup patch when titles will 
be adapted.
>> +
>> +            text.extend([
>> +                '\n'.join(desc_text),
>> +            ])
>> +
>> +
>> +        # developer information
> For me would be more obvious comment something like: "parse struct tst_test content".
>
> Kind regards,
> Petr
>
>> +        text.append('')
>> +        text.extend(_generate_setup_table(conf))
>> +        text.append('')
>> +
>> +        # small separator between tests
>> +        text.extend([
>> +            '',
>> +            '.. raw:: html',
>> +            '',
>> +            '    <hr>',
>> +            '',
>> +        ])
>> +
>> +    with open(output, 'w+', encoding='utf-8') as new_tests:
>> +        new_tests.write('\n'.join(text))
> ...