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

testutils: Automated testing environment for release tests using pytest #155

Merged
merged 3 commits into from
Jul 15, 2020

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jul 10, 2020

Contribution description

Third time's a charm: after #79 and some major overhaul of that framework to use (then) riotnode (now riotctrl) and pytest by @fjmolinas, I ported the latter to run with the new riotctrl python package and the in-tree riotctrl_shell module.

I also cleaned up the by now very complicated history of that effort and squashed a lot of commits. Since the most of the porting work was done by @fjmolinas (I just moved some code around and enhanced his code) I made him the main author of the work in the commit history, but added all co-authors to the respective commits for the changes they made.

Testing procedure

Run tox and the tests will be run (there are none at the moment, see #160, #161, #162, #163, #158, and #159).
Check-out #164 to run all tests.
There is also some more info in the README how to select specific groups of tests.

Self-tests are run by Github Actions and can be run locally with tox -- --self-test.

Issues/PRs references

Alternative to #79.

Required for #160, #161, #162, #163, #158, and #159

@miri64
Copy link
Member Author

miri64 commented Jul 10, 2020

Spec06 is still a bit flaky due to the custom ShellInteraction for the udp command, that's why they are marked as WIP and currently still skipped in the test execution.

@miri64
Copy link
Member Author

miri64 commented Jul 10, 2020

Fixed the issue with spec 6. One issue was just a wrong assertion not in line with the release-specs and the other was that the timeout for the udp command was selected too short.

testutils/iotlab.py Outdated Show resolved Hide resolved
Comment on lines +125 to +105
def pytest_keyboard_interrupt(excinfo):
# pylint: disable=C0301
"""
Called on KeyInterrupt

See: https://docs.pytest.org/en/stable/reference.html?highlight=hooks#_pytest.hookspec.pytest_keyboard_interrupt
""" # noqa: E501
for child in RUNNING_CTRLS:
child.stop_term()
for exp in RUNNING_EXPERIMENTS:
exp.stop()
Copy link
Member Author

Choose a reason for hiding this comment

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

Another thing that is flaky, even with this function, is cleanup. When the tests run uninterrupted, everything is cleaned up properly, but once one hits Ctrl+C during the tests some nodes and experiments might still be left running. Maybe someone with more experience with pytest can help here. @aabadie @fjmolinas?

Copy link
Member Author

Choose a reason for hiding this comment

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

(it seems that both commands wait a bit after doing the actual stopping while pytest is too impatient and just kills the waiting).

@miri64
Copy link
Member Author

miri64 commented Jul 10, 2020

Fixed the issue with spec 6. One issue was just a wrong assertion not in line with the release-specs and the other was that the timeout for the udp command was selected too short.

Mh nope, the timeout is allegedly still too short :-/. This doesn't seem right to me. It is calculated here from the input parameters:

if delay_ms:
timeout = math.ceil((delay_ms * count) / 1000) + 30
else:
timeout = (count / 1000) * 30

So there should be more than enough time to wait for the prompt. However, it does not seem to be and the framework just fires a timeout in

return self.cmd(
"udp send {dest_addr} {port} {payload} {count} {delay_us}"
.format(dest_addr=dest_addr, port=port, payload=payload,
count=count, delay_us=delay_ms * 1000),
timeout=timeout, async_=async_
)

Maybe the nodes don't really wait a delay_us between sends but much longer? :-/ This problem does not appear with the ping6 interaction.

@miri64
Copy link
Member Author

miri64 commented Jul 10, 2020

Maybe the nodes don't really wait a delay_us between sends but much longer?

Yeah seems like this. Instead of waiting 1s with delay_ms=1000 its more like 1.1s (since the app builds the whole package at every iteration this isn't surprising). Added up to 1000 packets that's 100sec the replwrap needs to wait longer for the prompt. I guess I make the timeout slack adaptive to count then.

@miri64
Copy link
Member Author

miri64 commented Jul 10, 2020

Added up to 1000 packets that's 100sec the replwrap needs to wait longer for the prompt. I guess I make the timeout slack adaptive to count then.

Yepp, that worked :-)

@miri64 miri64 mentioned this pull request Jul 10, 2020
81 tasks
@miri64 miri64 force-pushed the pytest-release-tests/feat/initial branch 2 times, most recently from f5d6a79 to 9c62c0b Compare July 10, 2020 15:56
@miri64
Copy link
Member Author

miri64 commented Jul 10, 2020

Squashed the last few commits regarding GitHub Actions into one. They are now working https://github.com/miri64/Release-Specs/actions/runs/164755513. For now, only the self-testing of the test framework is done and I plan to keep it that way for pull requests and pushes. If this gets merged I plan to provide a separate workflow to run release tests periodically (maybe weekly?).

@miri64
Copy link
Member Author

miri64 commented Jul 11, 2020

@miri64
Copy link
Member Author

miri64 commented Jul 11, 2020

https://miri64.github.io/Release-Specs/test-reports/test-report-rc1-2020-07-11-12-44.html

I used junit2html to convert the test-report.xml generated by pytest to an HTML file 😊

@miri64
Copy link
Member Author

miri64 commented Jul 11, 2020

With the latest round of commits the success rate should be much higher (also include some clean-up of code I was annoyed with). I'm just running the script again and will come back with the results.

@miri64 miri64 force-pushed the pytest-release-tests/feat/initial branch 2 times, most recently from c2ceb5e to 1176ee1 Compare July 11, 2020 19:22
@miri64
Copy link
Member Author

miri64 commented Jul 11, 2020

I'm just running the script again and will come back with the results.

See #156 (comment). There only seems to be an issue with asyncio now.

@miri64
Copy link
Member Author

miri64 commented Jul 11, 2020

See #156 (comment). There only seems to be an issue with asyncio now.

From that comment: does anybody know how to get the stdout of failed tests into the report XML?

@miri64
Copy link
Member Author

miri64 commented Jul 11, 2020

From that comment: does anybody know how to get the stdout of failed tests into the report XML?

Found it

@miri64 miri64 force-pushed the pytest-release-tests/feat/initial branch from 311c4a0 to aa40c38 Compare July 12, 2020 18:46
@miri64
Copy link
Member Author

miri64 commented Jul 12, 2020

I don't assume anyone looked at my weekend-shift work, so I just squashed it all together into one fixup commit per base commit :-)

@miri64
Copy link
Member Author

miri64 commented Jul 13, 2020

This PR is getting quite large. Shall I split it up in one for the framework and one for each spec (with a tracking PR including all release tests implemented so far to ease testing)?

@miri64 miri64 force-pushed the pytest-release-tests/feat/initial branch from 8f0441e to 266f1d7 Compare July 13, 2020 15:23
@miri64
Copy link
Member Author

miri64 commented Jul 13, 2020

This PR is getting quite large. Shall I split it up in one for the framework and one for each spec (with a tracking PR including all release tests implemented so far to ease testing)?

Squashed in anticipation of that.

@miri64 miri64 force-pushed the pytest-release-tests/feat/initial branch from 266f1d7 to 648a5d3 Compare July 13, 2020 15:51
@miri64 miri64 changed the title *: Automated testing environment for release tests testutils: Automated testing environment for release tests using pytest Jul 13, 2020
@miri64
Copy link
Member Author

miri64 commented Jul 13, 2020

Ok, I now removed all spec test implementations. It's still big, but there is a lot of ground work done here that is needed by the other PRs.

@jia200x
Copy link
Member

jia200x commented Jul 15, 2020

self tests pass and code-wise looks ok overall. I used #164 to run all tests and the base script seems to work.
Since this is a huge improvement in the direction of automated testing, I would be OK to go with it in the current state (we can patch the Ctrl + C issue in a follow up).

Will run again with the last commit and I would say we are ready to go

Copy link
Member

@jia200x jia200x left a comment

Choose a reason for hiding this comment

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

All good. ACK

@jia200x
Copy link
Member

jia200x commented Jul 15, 2020

please squash before merging :)

@jia200x
Copy link
Member

jia200x commented Jul 15, 2020

(we should maybe add commit lint to this repository too)

@miri64
Copy link
Member Author

miri64 commented Jul 15, 2020

(we should maybe add commit lint to this repository too)

Yes, but since there are a lot of legacy scripts still lingering, until everything is integrated, I would prefer to do that after all spec tests are merged (see 5a95f20).

fjmolinas and others added 3 commits July 15, 2020 15:17
Co-Authored-By: Jose Alamos <jose.alamos@haw-hamburg.de>
Co-Authored-By: Martine S. Lenders <m.lenders@fu-berlin.de>
Co-Authored-By: Francisco Molina <femolina@uc.cl>
@miri64 miri64 force-pushed the pytest-release-tests/feat/initial branch from 2d482e8 to cb6a644 Compare July 15, 2020 13:17
@miri64
Copy link
Member Author

miri64 commented Jul 15, 2020

Squashed.

@miri64 miri64 merged commit 298ac75 into RIOT-OS:master Jul 15, 2020
@miri64 miri64 deleted the pytest-release-tests/feat/initial branch July 15, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants