Re: [LTP] [PATCH 2/2] doc: Add ground rules page

[Petr Vorel <pvorel@suse.cz>]

Hi all,

4 typos below. With that fixed before merge:
Reviewed-by: Petr Vorel <pvorel@suse.cz>

Few small minor things below.
...
> +Always clean up, even on failure
> +--------------------------------
> +
> +Every test should leave the system as it found it: unmount, restore sysctls,
> +delete temp files/dirs, kill spawned processes, remove cgroups/namespaces,
> +detach loop devices, restore ulimits, etc. Cleanup must run on early-exit
> +paths too.

There is an exception on cleanup dynamic memory allocation before tst_brk(),
right? I keep forgetting this, but it's 1) it's a C memory cleanup (not a system
cleanup) 2) a corner case (probably not needed to mention).

> +
> +The test library can simplify cleanup greatly as there are various helpers such as:
> +
> +- :c:type:`tst_test.needs_tmpdir <tst_test>` that creates and deletes a temporary directory for the test

How about use syntax which people actually use in the code?
- :c:type:`.needs_tmpdir = 1 <tst_test>` that creates and deletes a temporary directory for the test

> +- :c:type:`tst_test.save_restore <tst_test>` that saves and restores /sys/ and /proc/ files
> +- :c:type:`tst_test.needs_device <tst_test>` sets up and tears down a block device for the test
> +- :c:type:`tst_test.restore_wallclock <tst_test>` that restores wall clock after the test
> +- :c:type:`tst_test.needs_cgroup_ctrls <tst_test>` sets up and cleans up cgroups for the test
> +- ...
Maybe instead "..." use: "And many more"?

Also (OT( I'm getting convinced that even we transform all headers into
kerneldoc comments and use examples from the old doc [1] we will probably need
some highlevel document similar to the old doc (something shorter than the old
doc, with links pointing to the header docs, pointing out the most important
things).

[1] https://github.com/linux-test-project/ltp/blob/master/doc/old/C-Test-API.asciidoc

> +
> +
> +Write portable code
> +-------------------
> +
> +Avoid nonstandard libc APIs when a portable equivalent exists; don’t assume
> +64-bit, page size, endianness, or particular tool versions.
> +
> +If the test is specific to a certain architecture, make sure that it at least
> +compiles at the rest of architectures and set the
> +:c:type:`tst_test.supported_archs <tst_test>`.
> +
> +This also applies to shell code where it's easy to use bash features that are
> +not available on other shell implementations, e.g. dash or busybox. Make sure
> +to stick to portable POSIX shell whenever possible.
> +
> +You can check for common mistakes, not only in portability, with our
> +'make check' tooling.
nit: ``make check`` tooling.
> +
> +
> +Split changed into well defined chunks
> +--------------------------------------
> +
> +When submitting patches make sure to split the work into small well-defined
> +chunks. Patches that touch many files or mix unrelated changes and features are
> +difficuilt to review and are likely to be detalyed or even ignored.
typo: difficuilt => difficult, detalyed => delayed (?)

> +
> +Aim for a single logical change per patch. Split more complex works into a
> +patch series where each patch:
> +
> +  - builds/compiles successfully.
> +  - keeps tests and tooling functional.
> +  - does not introduce intermediate breakage.
> +  - has a clear commit message to explain the change.
> +  - Significant changes need to be detailed in the cover letter.
> +
> +
> +Be careful when using AI tools
> +------------------------------
> +
> +AI tools can be useful for executing, summarizing, or suggesting approaches,
> +but they can also be confidently wrong and give an illusion of correctness.
> +Treat AI output as untrusted: verify claims against the code, documentation,
> +and actual behavior on a reproducer.
> +
> +Do not send AI-generated changes as raw patches. AI-generated diffs often
> +contain irrelevant churn, incorrect assumptions, inconsistent style, or subtle
> +bugs, which creates additional burden for maintainers to review and fix.
> +
> +Best practice is to write your own patches and have them reviewed by AI before
> +submitting them, which helps add beneficial improvements to your work.
> +
> +
> +Kernel features and RCs
> +-----------------------
> +
> +LTP tests or fixes for kernel changes that have not yet been released may be
> +posted to the LTP list for a review but they will not be be accepted until
> +respective kernel changes are released. Review of such changes is also
> +considered to be lower priority than rest of the changes. This is because
> +kernel changes especially in the early RC phase are volatile and could be
> +changed or reverted.
> +
> +These patchses should also add a [STAGING] keyword into the patch subject, e.g.
typo: patchses => patches

> +"Subject: [PATCH v1][STAGING] fanotify: add test for <feature> (requires v6.19-rc3)"
> +
> +In a case that a test for unrelased kernel is really needed to be merged we do
typo: unrelased => unreleased

Kind regards,
Petr

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp