Re: [PATCH v4 1/9] KVM: selftest: Create KVM selftest runner

Vipin Sharma <[email protected]> writes:

>
> [...snip...]
>
> +def setup_logging():
> +    class TerminalColorFormatter(logging.Formatter):
> +        reset = "\033[0m"
> +        red_bold = "\033[31;1m"
> +        green = "\033[32m"
> +        yellow = "\033[33m"
> +        blue = "\033[34m"
> +
> +        COLORS = {
> +            SelftestStatus.PASSED: green,
> +            SelftestStatus.NO_RUN: blue,
> +            SelftestStatus.SKIPPED: yellow,
> +            SelftestStatus.FAILED: red_bold
> +        }
> +
> +        def __init__(self, fmt=None, datefmt=None):
> +            super().__init__(fmt, datefmt)
> +
> +        def format(self, record):
> +            return (self.COLORS.get(record.levelno, "") +
> +                    super().format(record) + self.reset)
> +

The commit message above says the printing will be in colors when the
terminal supports it, but if I'm reading this correctly, the colors will
always be printed.

I was expecting something like if the output is piped to some file or
like "not TTY" then don't print colors.

Would you consider not printing in color at all? Or adding some kind of
"turn off all display magic" flag?

I also saw code for a sticky footer in another patch in this series,
that's probably not nice for other stuff wrapping this runner.

> +    logger = logging.getLogger("runner")
> +    logger.setLevel(logging.INFO)
> +
> +    ch = logging.StreamHandler()
> +    ch_formatter = TerminalColorFormatter(fmt="%(asctime)s | %(message)s",
> +                                          datefmt="%H:%M:%S")
> +    ch.setFormatter(ch_formatter)
> +    logger.addHandler(ch)
> +
> +
> +def fetch_testcases_in_dirs(dirs):
> +    testcases = []
> +    for dir in dirs:
> +        for root, child_dirs, files in os.walk(dir):
> +            for file in files:
> +                testcases.append(os.path.join(root, file))
> +    return testcases
> +
> +

Sean pointed me here from guest_memfd tests [1]. The Makefiles allow us
to generate test configs at build time. For [1], I could configure all
the test parameters statically or at build time.

[1] https://lore.kernel.org/all/[email protected]/

Perhaps for a future extension, but would you consider taking the output
of some program as input for cases to run?

My (future) use case is that with hugepages, I want to run something
like

  ./guest_memfd_test --order=0
  ./guest_memfd_test --order=9
  ./guest_memfd_test --order=18

And 0, 9 and 18 are the supported HugeTLB orders on the machine being
tested. I'd like to iterate over supported HugeTLB orders at runner
runtime instead of at build time.


> +def fetch_testcases(args):
> +    testcases = args.testcases
> +    testcases.extend(fetch_testcases_in_dirs(args.dirs))
> +    # Remove duplicates
> +    testcases = list(dict.fromkeys(testcases))
> +    return testcases
> +
> +
> +def main():
> +    args = cli()
> +    setup_logging()
> +    testcases = fetch_testcases(args)
> +    return TestRunner(testcases).start()
> +
> +
> +if __name__ == "__main__":
> +    PYTHON_VERSION = (3, 6)
> +    if sys.version_info < PYTHON_VERSION:
> +        print(f"Minimum required python version {PYTHON_VERSION}, found {sys.version}")
> +        sys.exit(1)
> +
> +    sys.exit(main())

This shouldn't block merge: why not align with the kernel's official
required python version?

>
> [...snip...]
>
> +class Selftest:
> +    """
> +    Represents a single selftest.
> +
> +    Extract the test execution command from test file and executes it.
> +    """
> +
> +    def __init__(self, test_path):
> +        test_command = pathlib.Path(test_path).read_text().strip()
> +        if not test_command:
> +            raise ValueError("Empty test command in " + test_path)
> +
> +        test_command = os.path.join(".", test_command)
> +        self.exists = os.path.isfile(test_command.split(maxsplit=1)[0])
> +        self.test_path = test_path
> +        self.command = test_command
> +        self.status = SelftestStatus.NO_RUN
> +        self.stdout = ""
> +        self.stderr = ""
> +
> +    def run(self):
> +        if not self.exists:
> +            self.stderr = "File doesn't exist."
> +            return
> +
> +        run_args = {
> +            "universal_newlines": True,
> +            "shell": True,
> +            "stdout": subprocess.PIPE,
> +            "stderr": subprocess.PIPE
> +        }
> +        proc = subprocess.run(self.command, **run_args)
> +
> +        out, err = proc.stdout, proc.stderr
> +        self.stdout = out.decode("utf-8", "replace") if isinstance(out, bytes) else (out or "")
> +        self.stderr = err.decode("utf-8", "replace") if isinstance(err, bytes) else (err or "")

I think it would be useful to capture stdout and stderr in order that it
was output, so that the output being saved shows everything
interleaved. I think the order could be useful in debugging.

I can see benefits in knowing which was on stdout and which was on
stderr too, so is there some way of having both?

> +
> +        if proc.returncode == 0:
> +            self.status = SelftestStatus.PASSED
> +        elif proc.returncode == 4:
> +            self.status = SelftestStatus.SKIPPED
> +        else:
> +            self.status = SelftestStatus.FAILED

Using this class-based pattern requires us to do
Selftest(test_path).run(). Would you consider using a function-based
pattern, like run_selftest(test_path) instead?

>
> [...snip...]
>

-- 
kvm-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/kvm-riscv