Skip to content

network: add tcp test#326

Open
adamdebek wants to merge 3 commits intomasterfrom
adamdebek/CI-343
Open

network: add tcp test#326
adamdebek wants to merge 3 commits intomasterfrom
adamdebek/CI-343

Conversation

@adamdebek
Copy link
Contributor

@adamdebek adamdebek commented Feb 20, 2024

JIRA: CI-343

Description

Motivation and Context

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.

@adamdebek adamdebek marked this pull request as draft February 20, 2024 18:36
@github-actions
Copy link

github-actions bot commented Feb 20, 2024

Unit Test Results

9 546 tests  +84   8 957 ✅ +84   56m 22s ⏱️ + 5m 9s
  564 suites + 3     589 💤 ± 0 
    1 files   ± 0       0 ❌ ± 0 

Results for commit b164401. ± Comparison against base commit bb76df5.

♻️ This comment has been updated with latest results.

@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 5 times, most recently from eb1c8c9 to dc3c265 Compare February 21, 2024 10:41
mateusz-bloch

This comment was marked as resolved.

@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 11 times, most recently from 52d08d7 to 9616f35 Compare February 21, 2024 13:44
@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 10 times, most recently from 4aac9fa to 146a39e Compare February 27, 2024 17:02
@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 2 times, most recently from 016d94e to fbb9e93 Compare April 8, 2024 15:00
@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 2 times, most recently from 11556e5 to d100fa7 Compare April 8, 2024 15:32
@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 3 times, most recently from a2706b4 to 893be2e Compare April 8, 2024 17:50
@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 4 times, most recently from cceda4f to 9ff1e37 Compare April 24, 2024 16:35
@adamdebek adamdebek force-pushed the adamdebek/CI-343 branch 4 times, most recently from 893453e to 0e753e4 Compare June 12, 2024 10:49
- introduce basic Shell class
- refactor ShellHarness
- add TestHarness
- change prompt checks to assert after each action instead of before

JIRA: CI-343
Comment on lines +160 to +196
if idx == 0:
if parsed["status"] in ["FAIL", "IGNORE"]:
last_assertion = parsed

elif idx in (1, 2):
if last_assertion.get("msg"):
parsed["msg"] = last_assertion["msg"]
last_assertion = {}

status = Status.from_str(parsed["status"])
subname = f"{parsed['group']}.{parsed['name']}"
if "path" in parsed and "line" in parsed:
parsed["msg"] = f"[{parsed['path']}:{parsed['line']}] " + parsed.get("msg", "")
result.add_subresult(subname, status, parsed.get("msg", ""))

stats[parsed["status"]] += 1
results.append(parsed)

elif idx == 3:
for k, v in parsed.items():
if k != "result":
parsed[k] = int(v)

assert (
parsed["total"] == sum(stats.values())
and parsed["fail"] == stats["FAIL"]
and parsed["ignore"] == stats["IGNORE"]
), "".join(("There is a mismatch between the number of parsed tests and overall results!\n",
"Parsed results from the final Unity message (total, failed, ignored): ",
f"{parsed['total']}, {parsed['fail']}, {parsed['ignore']}\n",
"Found test summary lines (total, failed, ignored): ",
f"{sum(stats.values())}, {stats['FAIL']}, {stats['IGNORE']}"))

break

elif idx == 4:
network_test_responder.do_cmd(parsed["cmd"])
Copy link
Contributor

Choose a reason for hiding this comment

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

match ... case could be used instead, but this is a purely stylistic decision

Copy link
Contributor Author

Choose a reason for hiding this comment

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

match case was introduced in python 3.10 and some of our raspberries use still python 3.9

Copy link
Member

Choose a reason for hiding this comment

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

we should be running inside gh-runner container (ubuntu 24.04 based) - so the python version should be at least 3.12.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I was running these tests locally during development, and debugging is also done locally. Despite that, I prefer using if statements rather than match case when there are only a few cases

Copy link
Contributor

@julianuziemblo julianuziemblo Jan 7, 2026

Choose a reason for hiding this comment

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

as I said: stylistic decision, fair to me

Copy link
Contributor

Choose a reason for hiding this comment

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

@damianloew given our previous discussion in the PR about compatibility that resulted in the decision to upgrade the OS on our rasp pi's, should we stop keeping the code compatible with python 3.9?

If so, I too would be in favour of turning the if/elif/else into match ... case here

Comment on lines +5 to +15
- name: tcp
execute: test_tcp en1
network_setup: True
targets:
value: [armv7a7-imx6ull-evk, armv7m7-imxrt106x-evk]

- name: tcp
execute: test_tcp en2
network_setup: True
targets:
value: [armv7m7-imxrt117x-evk]
Copy link
Contributor

Choose a reason for hiding this comment

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

why no im6ull en2 and imxrt117x en1 tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only used the interfaces that had a cable plugged in inside the rack units. I know the other interfaces differ in hardware on those targets, are they supported?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, they both are supported:

  • on imxrt117x: ENET is a 10/100M with rtl8201fi chip, ENET_1G is a 10/100/1000M with rtl8211fdi
  • on imx6ull: ENET1 and ENET2 are 10/100M with the same PHY ksz8081rnb

raise ParserError(f"network_setup must be a boolean value (true/false) not {network_setup}")

if network_setup:
self.test.iface_config = os.environ.get("IFACE_CONFIG", "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Question, is IFACE_CONFIG meant to be set by the CI (or user if run locally)?

suppress_dmesg: bool = True,
prompt: str
):
super().__init__()
Copy link
Contributor

Choose a reason for hiding this comment

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

I see no reason to have it here, this class doesn't have a parent class

# Setup environment for tests
def build_test(self, test: TestOptions) -> Callable[[TestResult], TestResult]:
builder = HarnessBuilder()
shell = Shell(self.dut, self.shell_prompt)
Copy link
Contributor

Choose a reason for hiding this comment

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

We import Shell only to create an object and parse it back to a harness. Perhaps, we should let the individual harnesses to handle it internally to reduce unnecessary imports.

I see the value in wrapping self.dut and self.shell_prompt into a single object, but if we don't use that object anywhere else, then I don't think we should be using it here at all

prompt_timeout: Optional timeout to wait before prompt will show up.

"""
class Shell():
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe instead of adding a shell object to each, we could make it an IntermediateShellHarness, a parent class of the harnesses below, that would inherit from IntermediateHarness as they do now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Either way, it looks like a change that is out of scope for this PR. We are making architectural changes with TestHarness and ShellHarness, which should be in a separate PR. Then, with those changes merged, we could implement NetworkSetupHarness as part of this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants