-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Removing unneeded get_test_info call #39967
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
base: master
Are you sure you want to change the base?
Removing unneeded get_test_info call #39967
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request prevents an error by skipping a call to get_test_info
for CommissionDeviceTest
. The feedback suggests using is not
for comparing class types.
src/python_testing/matter_testing_infrastructure/chip/testing/runner.py
Outdated
Show resolved
Hide resolved
…runner.py Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
PR #39967: Size comparison from 6d5aa8c to 51fdb21 Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
@@ -328,7 +328,11 @@ def run_tests_no_exit( | |||
CommissionDeviceTest.event_loop = event_loop | |||
test_class.event_loop = event_loop | |||
|
|||
get_test_info(test_class, matter_test_config) | |||
# If the test class is not CommissionDeviceTest, retrieve its information. | |||
# CommissionDeviceTest does not implement the methods expected by get_test_info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain why the clas does not implement the methods expected? Why is the fix to add a exception here (which is more tight coupling) rather than adding required methods to the test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a very good point and a great suggestion you've raised regarding the method implementation and coupling. Thanks for bringing it up.
Upon further investigation, I've actually uncovered an underlying issue. The get_test_info
call, when processing a CommissionDeviceTest
class, currently throws an error during the __init__
method of CommissionDeviceTest
itself. This seems to be a side effect of a recent refactoring within that class, specifically from PR #39478.
Given that the current behavior of CommissionDeviceTest
is causing this __init__
error due to the recent changes, I believe the most robust solution will require collaboration with the author of that original PR.
@jtrejoespinoza-grid Could you help me out with this, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andy31415 I don't have full context here, but besides adding the required tests, we also need to ensure there are values for default_controller
and matter_test_config
keys in those two assignments inside CommissionDeviceTest's
__init__
method.
if "default_controller" in test_config.user_params:
self.default_controller = test_config.user_params['default_controller']
if "matter_test_config" in test_config.user_params:
self.setup_payloads: List[SetupPayloadInfo] = get_setup_payload_info_config(
global_stash.unstash_globally(test_config.user_params['matter_test_config']))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably this is because before moving commissioning.py into its own file the class CommissionDeviceTest
was implemented in matter_testing.py
and this class used to extend from MatterBaseTest
. Currently on its own file it extends from mobly.base_test.BaseTestClass
this affects because it does not have all the methods implemented in the class MatterBaseTest
. This was done this way to avoid circular import. Probably some implementation on the class CommissionDeviceTest
should be done to clear this issues.
Hi @andy31415 and @jtrejoespinoza-grid Looking closer, I noticed the I tested removing it, and the code still behaves as expected. Do you see any issues with just removing this call? |
Not sufficiently familiar with this, but if you checked and our tests pass, we can remove |
I'll wait @jtrejoespinoza-grid 's feedback before proceeding with the change. |
Hi @rquidute regarding your comments I made some test by removing the line with the code : I re-built the python files again and the run some test with commissioning on-network with the Linux App and then commissioning ble-wifi with a M5 Stack flashed with Let me provide the information that I was able to gather. Then build python env:
Then load the env:
Run one test locally using commissioning on-network using the Linux app:
Logs: Run one test using a device with commissioning ble-wifi:
Logs : |
Thanks @jtrejoespinoza-grid |
PR #39967: Size comparison from 6d5aa8c to 336c36a Full report (59 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
CommissionDeviceTest
's get_test_info
calls
Summary
This PR removes the call
get_test_info(test_class, matter_test_config)
since it doesn't seem to be doing anything in this context.Related issues
project-chip/certification-tool-backend#242
Testing
Running the following command successfully:
python3 /root/python_testing/scripts/sdk/TC_CADMIN_1_11.py --commissioning-method on-network --trace-to json:log --discriminator 3840 --passcode 20202021
Running using TH client successfully (Commissioning):
python3 /root/python_testing/scripts/sdk/matter_testing_infrastructure/chip/testing/test_harness_client.py commission --trace-to json:log --commissioning-method on-network --discriminator 3840 --passcode 20202021 --storage_path /root/python_testing/admin_storage2.json
Running using TH client successfully (Test Execution):
python3 /root/python_testing/scripts/sdk/matter_testing_infrastructure/chip/testing/test_harness_client.py sdk/TC_ACE_1_3 TC_ACE_1_3 --tests test_TC_ACE_1_3 --trace-to json:log --in-test-commissioning-method on-network --discriminator 3840 --passcode 20202021