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

sanitycheck: Add a feature which can handle pytest script. #29427

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 19 additions & 1 deletion doc/guides/test/twister.rst
Expand Up @@ -329,7 +329,7 @@ harness: <string>
simple as a loopback wiring or a complete hardware test setup for
sensor and IO testing.
Usually pertains to external dependency domains but can be anything such as
console, sensor, net, keyboard, or Bluetooth.
console, sensor, net, keyboard, Bluetooth or pytest.

harness_config: <harness configuration options>
Extra harness configuration options to be used to select a board and/or
Expand Down Expand Up @@ -369,6 +369,11 @@ harness_config: <harness configuration options>

Only one fixture can be defined per testcase.

handle_script: <pytest dirctory> (default pytest)
Specify a pytest directory which need to excute when test case begin to running,
default pytest directory name is pytest, after pytest finished, twister will
check if this case pass or fail according the pytest report.

The following is an example yaml file with a few harness_config options.

::
Expand All @@ -390,6 +395,19 @@ harness_config: <harness configuration options>
tags: sensors
depends_on: i2c

The following is an example yaml file with pytest harness_config options, default pytest_root
name will be used if pytest_root not specified and the default pytest_timeout is 15 seconds.
as an example, test case in test/pytest_sample/ use the default value of these two options.

::

tests:
pytest.example:
harness: pytest
harness_config:
pytest_root: [pytest directory name]
pytest_timeout: [timeout]

filter: <expression>
Filter whether the testcase should be run by evaluating an expression
against an environment containing the following values:
Expand Down
69 changes: 69 additions & 0 deletions scripts/pylib/twister/harness.py
@@ -1,8 +1,12 @@
# SPDX-License-Identifier: Apache-2.0
import re
import os
import subprocess
from collections import OrderedDict
import xml.etree.ElementTree as ET

result_re = re.compile(".*(PASS|FAIL|SKIP) - (test_)?(.*)")
pytest_running_dir = ""

class Harness:
GCOV_START = "GCOV_COVERAGE_DUMP_START"
Expand All @@ -28,6 +32,10 @@ def __init__(self):
self.recording = []
self.fieldnames = []
self.ztest = False
self.running_dir = None
self.source_dir = None
self.pytest_root = 'pytest'
self.pytest_timeout = 15
Comment on lines +35 to +38
Copy link
Member

Choose a reason for hiding this comment

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

these are not needed by default, so why initializing them here? Your class Pytest inherits from Harness. You can overcharge the __init__ method in Pytest by calling the parent __init__ and adding extra stuff you need. This will help in keeping the modularity and coherence of the code
https://appdividend.com/2019/01/22/python-super-function-example-super-method-tutorial/

Copy link
Collaborator

Choose a reason for hiding this comment

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

updated in #33227, move to pytest.configure()


def configure(self, instance):
config = instance.testcase.harness_config
Expand All @@ -41,6 +49,10 @@ def configure(self, instance):
self.repeat = config.get('repeat', 1)
self.ordered = config.get('ordered', True)
self.record = config.get('record', {})
self.running_dir = config.get('running_dir', None)
Copy link
Member

Choose a reason for hiding this comment

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

why are we defining all of this, we know where we are running and where the source directory is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #33227

self.source_dir = config.get('source_dir', None)
self.pytest_root = config.get('pytest_root', 'pytest')
self.pytest_timeout = config.get('pytest_timeout', 15)
Comment on lines +52 to +55
Copy link
Member

Choose a reason for hiding this comment

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

As above. You can overwrite the configure() method in Pytest class. You can see below, in Console(Harness) how this is done there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

updated in #33227, move to pytest.configure()

Copy link
Member

Choose a reason for hiding this comment

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

why introducing this new timeout, the test scenario already has a timeout.

Copy link
Collaborator

Choose a reason for hiding this comment

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

removed in #33227


def process_test(self, line):

Expand Down Expand Up @@ -121,6 +133,63 @@ def handle(self, line):

self.process_test(line)

class Pytest(Harness):

def handle(self, line):
''' To keep artifacts of pytest in self.running_dir, pass this directory
by "--cmdopt". On pytest end, add a command line option and provide
the cmdopt through a fixture function
pytest handle output as a whole, it's not necessary to handle it
line by line
'''
global pytest_running_dir
Copy link
Member

Choose a reason for hiding this comment

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

global? AFAIK it is highly advised not to use global variables as this can lead to a lot of mess in the software and problems with debugging. It seems you even never use it anywhere else, so why the global here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I need a variable to remember pytest has been called for this case, so I use pytest_running_dir to identify this directory has been visited then pytest has been called. In C I could use "static" variable but in python I couldn't find an equivalence, so I use "global pytest_running_dir"

if self.running_dir is pytest_running_dir:
return
Copy link
Member

Choose a reason for hiding this comment

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

I have no idea what this logic suppose to do. Why would you return like that just at the beginning of handle()?

Copy link
Collaborator

Choose a reason for hiding this comment

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

To make sure this handle be called only once. Every line output from console will cause this handle be called. I need a variable to remember pytest has been called for this case, so I use pytest_running_dir to identify this directory has been visited then pytest has been called. In C I could use "static" variable but in python I couldn't find an equivalence, so I use
"global pytest_running_dir"


pytest_running_dir = self.running_dir
cmd = [
'pytest',
'-s',
os.path.join(self.source_dir, self.pytest_root),
'--cmdopt',
self.running_dir,
'--junit-xml',
os.path.join(self.running_dir, 'report.html'),
Copy link
Member

Choose a reason for hiding this comment

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

report.xml

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #33227

'-q'
]

with subprocess.Popen(cmd) as proc:
Copy link
Member

Choose a reason for hiding this comment

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

you should pipe the stdout and stderr so the messages from tests are not printed to the output when not needed (no verbose used)

Copy link
Collaborator

Choose a reason for hiding this comment

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

updated in #33227

Copy link
Member

Choose a reason for hiding this comment

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

this will generate lots of noise

i9:pytest_sample((HEAD detached from upstream/pull/29427)): twister -p qemu_x86 -T .
INFO    - Zephyr version: zephyr-v2.5.0-838-gba4a8fc61df2
INFO    - JOBS: 20
INFO    - Using 'zephyr' toolchain.
INFO    - Building initial testcase list...
INFO    - 1 test scenarios (1 configurations) selected, 0 configurations discarded due to filters.
INFO    - Adding tasks to the queue...
INFO    - Added initial list of jobs to queue
handle cmdopt:
/home/nashif/Work/zephyrproject/zephyr/tests/pytest/pytest_sample/twister-out/qemu_x86/tests/pytest/pytest_sample/pytest.sample
run test cases in:
/home/nashif/Work/zephyrproject/zephyr/tests/pytest/pytest_sample/twister-out/qemu_x86/tests/pytest/pytest_sample/pytest.sample
.
- generated xml file: /home/nashif/Work/zephyrproject/zephyr/tests/pytest/pytest_sample/twister-out/qemu_x86/tests/pytest/pytest_sample/pytest.sample/report.html -
1 passed in 0.01s
INFO    - Total complete:    1/   1  100%  skipped:    0, failed:    0
INFO    - 1 of 1 test configurations passed (100.00%), 0 failed, 0 skipped with 0 warnings in 19.91 seconds
INFO    - In total 1 test cases were executed, 0 skipped on 1 out of total 330 platforms (0.30%)
INFO    - 1 test configurations executed on platforms, 0 test configurations were only built.
INFO    - Saving reports...
INFO    - Writing xunit report /home/nashif/Work/zephyrproject/zephyr/tests/pytest/pytest_sample/twister-out/twister.xml...
INFO    - Writing xunit report /home/nashif/Work/zephyrproject/zephyr/tests/pytest/pytest_sample/twister-out/twister_report.xml...
INFO    - Run completed

We do not want to see pytest intermediate results in the twister output.

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #33227

try:
proc.wait(self.pytest_timeout)
tree = ET.parse(os.path.join(self.running_dir, "report.html"))
root = tree.getroot()
for child in root:
if child.tag == 'testsuite':
if child.attrib['failures'] != '0':
self.state = "failed"
elif child.attrib['skipped'] != '0':
self.state = "skipped"
elif child.attrib['errors'] != '0':
self.state = "errors"
else:
self.state = "passed"
except subprocess.TimeoutExpired:
proc.kill()
self.state = "failed"
except ET.ParseError:
self.state = "failed"
except IOError:
self.state = "failed"

if self.state == "passed":
self.tests[self.id] = "PASS"
elif self.state == "skipped":
self.tests[self.id] = "SKIP"
else:
self.tests[self.id] = "FAIL"

self.process_test(line)

class Test(Harness):
RUN_PASSED = "PROJECT EXECUTION SUCCESSFUL"
RUN_FAILED = "PROJECT EXECUTION FAILED"
Expand Down
9 changes: 8 additions & 1 deletion scripts/pylib/twister/twisterlib.py
Expand Up @@ -509,6 +509,8 @@ def handle(self):
harness_import = HarnessImporter(harness_name)
harness = harness_import.instance
harness.configure(self.instance)
harness.running_dir = self.build_dir
harness.source_dir = self.sourcedir
Comment on lines +512 to +513
Copy link
Member

Choose a reason for hiding this comment

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

Why you've added this to all already existing handlers, where it is not needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

updated in #33227, move to pytest.configure()


if self.call_make_run:
command = [self.generator_cmd, "run"]
Expand Down Expand Up @@ -789,6 +791,8 @@ def handle(self):
harness_import = HarnessImporter(harness_name)
harness = harness_import.instance
harness.configure(self.instance)
harness.running_dir = self.build_dir
harness.source_dir = self.sourcedir
Comment on lines +794 to +795
Copy link
Member

Choose a reason for hiding this comment

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

Why you've added this to all already existing handlers, where it is not needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

updated in #33227, move to pytest.configure()

read_pipe, write_pipe = os.pipe()
start_time = time.time()

Expand Down Expand Up @@ -1053,6 +1057,9 @@ def handle(self):
harness_import = HarnessImporter(self.instance.testcase.harness.capitalize())
harness = harness_import.instance
harness.configure(self.instance)
harness.running_dir = self.build_dir
harness.source_dir = self.sourcedir
Comment on lines +1060 to +1061
Copy link
Member

Choose a reason for hiding this comment

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

Why you've added this to all already existing handlers, where it is not needed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

updated in #33227


self.thread = threading.Thread(name=self.name, target=QEMUHandler._thread,
args=(self, self.timeout, self.build_dir,
self.log_fn, self.fifo_fn,
Expand Down Expand Up @@ -1757,7 +1764,7 @@ def __lt__(self, other):
def testcase_runnable(testcase, fixtures):
can_run = False
# console harness allows us to run the test and capture data.
if testcase.harness in [ 'console', 'ztest']:
if testcase.harness in [ 'console', 'ztest', 'pytest']:
can_run = True
# if we have a fixture that is also being supplied on the
# command-line, then we need to run the test, not just build it.
Expand Down
12 changes: 12 additions & 0 deletions scripts/schemas/twister/testcase-schema.yaml
Expand Up @@ -62,6 +62,12 @@ mapping:
"repeat":
type: int
required: no
"pytest_root":
type: str
required: no
"pytest_timeout":
Copy link
Member

Choose a reason for hiding this comment

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

no need for a new timeout, use the existing one

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed

type: int
required: no
"regex":
type: seq
required: no
Expand Down Expand Up @@ -187,6 +193,12 @@ mapping:
"repeat":
type: int
required: no
"pytest_root":
type: str
required: no
"pytest_timeout":
type: int
required: no
"regex":
type: seq
required: no
Expand Down
7 changes: 7 additions & 0 deletions tests/pytest/pytest_sample/CMakeLists.txt
@@ -0,0 +1,7 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.13.1)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(pytest_sample)

target_sources(app PRIVATE src/main.c)
2 changes: 2 additions & 0 deletions tests/pytest/pytest_sample/prj.conf
@@ -0,0 +1,2 @@
CONFIG_ZTEST=y
CONFIG_IDLE_STACK_SIZE=4096
14 changes: 14 additions & 0 deletions tests/pytest/pytest_sample/pytest/conftest.py
@@ -0,0 +1,14 @@
# Copyright (c) 2020 Intel Corporation.
#
# SPDX-License-Identifier: Apache-2.0

import pytest

def pytest_addoption(parser):
parser.addoption(
'--cmdopt'
)

@pytest.fixture()
def cmdopt(request):
return request.config.getoption('--cmdopt')
24 changes: 24 additions & 0 deletions tests/pytest/pytest_sample/pytest/test_ctf.py
@@ -0,0 +1,24 @@
# Copyright (c) 2020 Intel Corporation.
#
# SPDX-License-Identifier: Apache-2.0
Copy link
Member

Choose a reason for hiding this comment

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

test_ctf? need another file name.

Copy link
Collaborator

Choose a reason for hiding this comment

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

fixed in #33227


import os
import pytest

@pytest.fixture(autouse=True)
def pytest_cmdopt_handle(cmdopt):
''' handle cmdopt '''
print("handle cmdopt:")
print(cmdopt)
data_path = cmdopt
os.environ['data_path'] = str(data_path)

def test_case(cmdopt):
''' test case '''
if not os.path.exists(cmdopt):
assert 0
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

why not just:

assert os.path.exists(cmdopt)

?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you, I've updated it in #33227

print("run test cases in:")
print(cmdopt)

if __name__ == "__main__":
pytest.main()
19 changes: 19 additions & 0 deletions tests/pytest/pytest_sample/src/main.c
@@ -0,0 +1,19 @@
/*
* Copyright (c) 2020 Intel Corporation
*
* SPDX-License-Identifier: Apache-2.0
*/
#include <ztest.h>
void test_pytest(void)
{
TC_PRINT("Hello world\n");
}

void test_main(void)
{
ztest_test_suite(test_pytest,
ztest_unit_test(test_pytest)
);

ztest_run_test_suite(test_pytest);
}
Comment on lines +1 to +19
Copy link
Member

Choose a reason for hiding this comment

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

@nashif Were you able to follow the design of this "feature"? It builds and executes some hello_word sample, which is in fact not tested. It is just a "workaround" to fit the existing structure of a workflow, where something has to be built. Then the sample is executed and the output line is "handled". In fact, the line is not checked but serves as a trigger for calling pytest. The test for pytest has nothing to do with the building sample.

4 changes: 4 additions & 0 deletions tests/pytest/pytest_sample/testcase.yaml
@@ -0,0 +1,4 @@
tests:
Copy link
Member

Choose a reason for hiding this comment

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

move this under tests/testsuite/pytest

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't push to youhua's repo any more, I update this PR to #33227

pytest.sample:
platform_allow: qemu_x86 qemu_x86_64
Copy link
Member

Choose a reason for hiding this comment

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

This causes the same test to be run 2x. As I understand, the platform chosen has no impact on the actual test. It just serves as a stub so sth is build and handling lines can trigger calling pytest. I would say that there should be just a single "native_posix" if we accept this workflow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

updated in #33227

harness: pytest