mbedtls|busybox: fix measuring test duration (trunner API change approach) #284

nalajcie · 2023-10-26T10:22:23Z

Description

mbedtls: fix measuring test duration
The subtest result was incorrectly "finished" (added to result) one line after
the result (so actually computed the time of the next test).

Use simplified API to add optional test message to the correct subtest
trunner: make subresult API more robust
- remove add_subresult_obj() as it's misleading (doesn't add by reference,
  just by value
- make add_subresult() return the subresult object which can be updated at a
  later time

Motivation and Context

phoenix-rtos/phoenix-rtos-project#877

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

nalajcie · 2023-10-26T10:24:56Z

switching to draft until busybox harness changes would be implemented

github-actions · 2023-10-26T10:36:53Z

Unit Test Results

5 989 tests ±0 5 348 ✔️ ±0 31m 29s ⏱️ + 2m 2s
  333 suites ±0   641 💤 ±0
      1 files ±0       0 ❌ ±0

Results for commit eaf18cf. ± Comparison against base commit 36b0b78.

♻️ This comment has been updated with latest results.

busybox/busybox.py

nalajcie · 2023-10-27T09:47:35Z

busybox/busybox.py

@@ -26,21 +26,23 @@ def harness(dut: Dut, ctx: TestContext, result: TestResult):
        parsed = dut.match.groupdict()

        if idx == 2:
-            msg.append("\t\t" + parsed["line"])
-            continue
+            line_msg = parsed["line"] + " "


I don't really know how the full output from the succesful/failed tescases looks like. Maybe we can put sample output as a mulitiline variable / comment after imports (to be used for pytest harness unit test in the near future hopefully).

From your code I suspect the output might be:

FAIL: test-name-1 why-failed-line-1 why-failed-line-2 msg-for-test-2 msg-for-test-2 PASS: test-name-2

If so - we can't assign message lines to the testcases in any sensible way (your code would append all 4 lines to test-name-1, but that's ok if the input is really like that) - but we should mention in comment that the messages might be invalid in that case.

After checking, the output can appear in several ways. It might be produced during the test. In the event of a failure, lines can be printed both during the test when something fails and after the test. However, when printing strictly informational messages, they are preceded by ====================== to distinguish them.

e.g.:

I've checked the busybox testsuite and it seems seems to be true only for newstyle tests (oldstyle tests don't print === nor messages before the test result - see mkdir for example). I don't see how to distinguish them in harness, maybe we should patch busybox testsuite framework instead?

damianloew

Tested some of the most vulnerable cases and it seems to work really good (both mbedtls and busybox). Let's wait for phoenix-rtos/phoenix-rtos-ports#69 merge

busybox/busybox.py

mbedtls/mbedtls.py

busybox/busybox.py

nalajcie · 2023-11-10T14:52:28Z

busybox/busybox.py

-from trunner.types import TestResult, TestSubResult, Status
+from trunner.types import TestResult, Status
+
+# Example of parsed outputs:


use multiline string , right now I don't know if the input has spaces in front or not, eg:

EXAMPLE_INPUT = """ test-msg-line-1 test-msg-line-2 FAIL: test-name-1 fail-msg-line-1 fail-msg-line-2 ---Testcase test-name-2 starting--- test-msg-line-1 test-msg-line-2 PASS: test-name-2 ---Testcase test-name-3 starting--- SKIPPED: test-name-3 """

this would also make it easier to write unit tests (use variable as an input).

Also - test names should be in '?

busybox/busybox.py

damianloew

Thanks for the latest improvements. Just 2 things more

damianloew · 2023-11-13T14:56:21Z

busybox/busybox.py

+EXAMPLE_INPUT = """
+---Testcase 'busybox as unknown name' starting---
+echo -ne '' >input
+echo -ne '' | ./unknown 2>&1
+PASS: busybox as unknown name
+---Testcase 'date-@-works' starting---
+FAIL: date-@-works


I think that it would be better to place it in a comment.

I based on the previous comment:

Please keep it as a multiline variable (reasoning provided previously).

Oh, right. Ok, I was worried about some potential issues (flake and so on) when we don't use those variables in fact.

busybox/busybox.py

damianloew

@mateusz-bloch please add JIRA ID to the first two commits and I'd like to merge it :)

- remove add_subresult_obj() as it's misleading (doesn't add by reference, just by value - make add_subresult() return the subresult object which can be updated at a later time Fixes: phoenix-rtos/phoenix-rtos-project#877 JIRA: CI-384

The subtest result was incorrectly "finished" (added to result) one line after the result (so actually computed the time of the next test). Use simplified API to add optional test message to the correct subtest Fixes: phoenix-rtos/phoenix-rtos-project#877 JIRA: CI-384

damianloew

LGTM

The subtest result was incorrectly finished (added to result) one line after the result (so actually computed the time of the next test). JIRA: CI-384

JIRA: CI-384

damianloew

One case from busybox test suite was not handled, thanks for adding it @mateusz-bloch. Looks good to me

nalajcie requested review from mateusz-bloch and damianloew October 26, 2023 10:22

nalajcie marked this pull request as draft October 26, 2023 10:23

nalajcie force-pushed the nalajcie/trunner branch from 9cb3aaf to 54f39f7 Compare October 26, 2023 10:24

mateusz-bloch force-pushed the nalajcie/trunner branch from 0569db4 to 6790609 Compare October 26, 2023 12:02

nalajcie commented Oct 27, 2023

View reviewed changes

mateusz-bloch force-pushed the nalajcie/trunner branch from 6790609 to 2857ef5 Compare October 27, 2023 11:35

mateusz-bloch force-pushed the nalajcie/trunner branch from 2857ef5 to d0d2298 Compare November 7, 2023 19:00

damianloew reviewed Nov 8, 2023

View reviewed changes

mateusz-bloch force-pushed the nalajcie/trunner branch from d0d2298 to 3779256 Compare November 8, 2023 12:35

damianloew requested changes Nov 8, 2023

View reviewed changes

busybox/busybox.py Outdated Show resolved Hide resolved

busybox/busybox.py Show resolved Hide resolved

mateusz-bloch force-pushed the nalajcie/trunner branch 5 times, most recently from 004691d to 3dc6192 Compare November 10, 2023 12:33

nalajcie commented Nov 10, 2023

View reviewed changes

mateusz-bloch force-pushed the nalajcie/trunner branch from 3dc6192 to 616609d Compare November 13, 2023 14:45

github-actions bot reviewed Nov 13, 2023

View reviewed changes

busybox/busybox.py Show resolved Hide resolved

busybox/busybox.py Show resolved Hide resolved

mateusz-bloch force-pushed the nalajcie/trunner branch from 616609d to 5af9e03 Compare November 13, 2023 14:50

mateusz-bloch requested a review from damianloew November 13, 2023 14:51

damianloew requested changes Nov 13, 2023

View reviewed changes

mateusz-bloch force-pushed the nalajcie/trunner branch 2 times, most recently from f793544 to f2592cb Compare November 13, 2023 15:11

damianloew reviewed Nov 14, 2023

View reviewed changes

nalajcie added 2 commits November 14, 2023 14:07

trunner: make subresult API more robust

5ef2b6c

- remove add_subresult_obj() as it's misleading (doesn't add by reference, just by value - make add_subresult() return the subresult object which can be updated at a later time Fixes: phoenix-rtos/phoenix-rtos-project#877 JIRA: CI-384

mateusz-bloch force-pushed the nalajcie/trunner branch from f2592cb to 40885da Compare November 14, 2023 13:11

damianloew marked this pull request as ready for review November 14, 2023 15:17

damianloew previously approved these changes Nov 14, 2023

View reviewed changes

mateusz-bloch dismissed damianloew’s stale review via 61d9f82 November 14, 2023 16:27

mateusz-bloch force-pushed the nalajcie/trunner branch from 40885da to 61d9f82 Compare November 14, 2023 16:27

mateusz-bloch requested a review from damianloew November 14, 2023 16:28

mateusz-bloch added 2 commits November 14, 2023 17:54

busybox: fix measuring test duration

287bc4f

The subtest result was incorrectly finished (added to result) one line after the result (so actually computed the time of the next test). JIRA: CI-384

trunner/busybox: add UNTESTED to Status

eaf18cf

JIRA: CI-384

mateusz-bloch force-pushed the nalajcie/trunner branch from 61d9f82 to eaf18cf Compare November 14, 2023 16:55

damianloew approved these changes Nov 14, 2023

View reviewed changes

damianloew merged commit bc832db into master Nov 16, 2023
33 checks passed

damianloew deleted the nalajcie/trunner branch November 16, 2023 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mbedtls|busybox: fix measuring test duration (trunner API change approach) #284

mbedtls|busybox: fix measuring test duration (trunner API change approach) #284

nalajcie commented Oct 26, 2023

nalajcie commented Oct 26, 2023

github-actions bot commented Oct 26, 2023 •

edited

Loading

nalajcie Oct 27, 2023

mateusz-bloch Oct 27, 2023

nalajcie Oct 27, 2023

damianloew left a comment

nalajcie Nov 10, 2023

damianloew left a comment

damianloew Nov 13, 2023

mateusz-bloch Nov 13, 2023

nalajcie Nov 13, 2023

damianloew Nov 13, 2023

damianloew left a comment

damianloew left a comment

damianloew left a comment

mbedtls|busybox: fix measuring test duration (trunner API change approach) #284

mbedtls|busybox: fix measuring test duration (trunner API change approach) #284

Conversation

nalajcie commented Oct 26, 2023

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

nalajcie commented Oct 26, 2023

github-actions bot commented Oct 26, 2023 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

damianloew left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 26, 2023 •

edited

Loading