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

[Tooling] Make IP/Port for "Accessory Server Bridge" configurable for test runs #29301

Closed
Apollon77 opened this issue Sep 16, 2023 · 5 comments
Closed

Comments

@Apollon77
Copy link
Contributor

Reproduction steps / Feature

Before the tests were removed from chip-tool the logic of the "Accessory Server Bridge" could easiely changed by just changing the files in

https://github.com/project-chip/connectedhomeip/tree/master/src/app/tests/suites/commands/system/scripts

With the new way to run tests introduced by #28783 this now is much more "integrated" and hard coded.

To use the test suites for automated device testing by vendors or by matter implementations like https://github.com/project-chip/matter.js to proof compliance (which was easy to achieve with the old way to run the tests) this is now very complicated because the "websocket-server IP and PORTs are hard coded insid ethe test runners.

https://github.com/project-chip/connectedhomeip/blob/master/scripts/py_matter_yamltests/matter_yamltests/pseudo_clusters/clusters/accessory_server_bridge.py#L20-L24

It would be great and a huge benefit if this would be configurable and not hard coded (and also specially set to a 10ish-IP when running on Linux). Config could be done via a parameter or an environment variable - whatever fits better ...

I would do a PR but pything is not really my home base ;-)

Thank you

Platform

other

Platform Version(s)

No response

Type

YAML tested

(Optional) If manually tested please explain why this is only manually tested

No response

Anything else?

No response

@bzbarsky-apple
Copy link
Contributor

@Apollon77 I'm a little confused. Before, this was hardcoded in a bunch of places. Now it's still hardcoded, but in one place, which seems like it would be easier to change.... Why is the new way more of a problem than the old one?

But in general, that whole setup is meant to apply to Matter CI, which is why it has the IPs it has: in CI the server accessory is in fact running on localhost or at 10.10.10.5 on Linux, due to these bits in the CI test runner:

scripts/tests/chiptest/accessories.py:    IP = '10.10.10.5'
scripts/tests/chiptest/linux.py:        "ip addr add 10.10.10.5/24 dev eth-ci",
scripts/tests/chiptest/linux.py:        "ip addr del 10.10.10.5/24 dev eth-ci",

@Apollon77
Copy link
Contributor Author

The old places where executed as "shell like pythong scripts" relative to where the chip-tool binary was places. So you could easiely create a "Reboot.py" which was no python anymore but a simple shell script. And in fact beside a chip-tool binary and these files in a defined folder structure you needed nothing to execute the tests.
We were building connectedhomeip repo and bins just once and cached the generated bins on github action cache. ALl subsequent runs just used the binary from before.

The new place is integrated in the whole "python yaml tests runner framework", so I would need to patch this one file after checking out the "fill connectedhomeip repo".

I also found that other "linux.py" code in the meantime, so maybe it is not that bad and maybe i do not need to replace it when I can make sure to run that logic before to to spin up this IP ...

Maybe with docs that all would be more clear, so I need to guess and ask ... sorry for that

@Apollon77
Copy link
Contributor Author

I worked myself into your python test stuff and I think I can utilize it from how it is there. So I close here for now.

(I now just need to find all places where the testing framework waits for special log Text parts from Chip/apps ;-))

@Apollon77 Apollon77 closed this as not planned Won't fix, can't repro, duplicate, stale Sep 17, 2023
@vivien-apple
Copy link
Contributor

Hi @Apollon77, if you want to update the method at line

to consume an environment variable if available that if fine.

It may look similar to something like the following:

diff --git a/scripts/py_matter_yamltests/matter_yamltests/pseudo_clusters/clusters/accessory_server_bridge.py b/scripts/py_matter_yamltests/matter_yamltests/pseudo_clusters/clusters/accessory_server_bridge.py
index dce6518776..ec35a1af19 100644
--- a/scripts/py_matter_yamltests/matter_yamltests/pseudo_clusters/clusters/accessory_server_bridge.py
+++ b/scripts/py_matter_yamltests/matter_yamltests/pseudo_clusters/clusters/accessory_server_bridge.py
@@ -13,6 +13,7 @@
 #    See the License for the specific language governing permissions and
 #    limitations under the License.
 
+import os
 import sys
 import xmlrpc.client
 
@@ -25,7 +26,9 @@ if sys.platform == 'linux':
 
 
 def _make_url():
-    return f'http://{_IP}:{_PORT}/'
+    ip_address = os.environ.get('CHIPTEST_WS_IP', _IP)
+    port = os.environ.get('CHIPTEST_WS_PORT', _PORT)
+    return f'http://{ip_address}:{port}/'

That should prevent you to patch the repo by just using a dedicated address/port as env variable as you suggest.

@Apollon77
Copy link
Contributor Author

Thank you, I will consider it if needed. In fact I currently try around with using the "full"framework from you and try to use a nodejs script directly as "app binary to start" ... because then the IP tear-up/down could be used as is :-) I will play around with that soon

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

No branches or pull requests

3 participants