TC-FAN-3.1 - Update: Enhance attribute value testing #36971
Draft
+348
−110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with |
github-actions
bot
added
tests
matter-1.4-te2-script-change
|
PR #36971: Size comparison from f8d457a to 6c96a7d Full report (54 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36971: Size comparison from f8d457a to 62f7df7 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36971: Size comparison from f8d457a to 643c405 Increases above 0.2%:
Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
reviewed
Jan 9, 2025
andy31415
reviewed
Jan 9, 2025
andy31415
reviewed
Jan 9, 2025
andy31415
reviewed
Jan 9, 2025
…eip into TC-FAN-3.1
|
PR #36971: Size comparison from 207cdfa to f171cfc Full report (72 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36971: Size comparison from 207cdfa to bcf08d8 Full report (3 builds for cc32xx, stm32)
|
|
PR #36971: Size comparison from 7c1d6f7 to 6ce438e Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
|
PR #36971: Size comparison from 7c1d6f7 to e000d0f Full report (5 builds for cc32xx, stm32, tizen)
|
|
PR #36971: Size comparison from 7c1d6f7 to e992e64 Full report (69 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nxp, psoc6, qpg, stm32, telink, tizen)
|
raul-marquez-csa
changed the title
raul-marquez-csa
requested review from
cecille,
andy31415,
gvargas-csa,
j-ororke and
p0fi
cecille
reviewed
Feb 19, 2025
src/python_testing/TC_FAN_3_1.py
Outdated
| sub_text = f"({value_previous}:{value_previous.name}) to ({value_current}:{value_current.name})" | ||
| logging.info(f"\t\t[FC] {attr_to_verify.__name__} changed from {sub_text}") | ||
|
|
||
| async def log_scenario(self, endpoint, attr_to_update, attr_to_verify, speed_setting_read, order): |
There was a problem hiding this comment.
why does the speed setting get passed in, but the other settings are read as a part of the logging?
There was a problem hiding this comment.
I updated log_scenario to not read the attributes, all attribute values are passed in now as they were already available.
src/python_testing/TC_FAN_3_1.py
Outdated
|
|
||
| fan_modes = None | ||
| if fan_mode_sequence == 0: | ||
| fan_modes = [fm_en.kOff.value, fm_en.kLow.value, fm_en.kMedium.value, fm_en.kHigh.value] |
There was a problem hiding this comment.
These are IntEnums, I think you should be able to use these directly, without the .value
There was a problem hiding this comment.
Correct, updated to use enums as is
src/python_testing/TC_FAN_3_1.py
Outdated
| return None | ||
|
|
||
| @staticmethod | ||
| def get_enum_value(value) -> int: |
There was a problem hiding this comment.
do you actually need this? What was happening with the enum values? They're MatterIntEnum, so they should be able to just be used as int values.
There was a problem hiding this comment.
Correct, removed get_enum_value function
| ] | ||
|
|
||
| @staticmethod | ||
| async def get_attribute_value_from_queue(queue, attribute, timeout_sec: float) -> Any: |
There was a problem hiding this comment.
You're using the ClusterAttributeChangeAccumulator, so there's a function that does this already - await_all_final_values_reported
There was a problem hiding this comment.
I don't think await_all_final_values_reported can help me in this scenario.
Let's take the updating PercentSetting and monitoring FanMode scenario.
- We have no way of knowing before hand which
PercentSettingvalue will trigger aFanModechange - This is run iteratively, so not all
PercentSettingwritten values will trigger aFanModechange - Once we detect a
FanModechange, we need the attribute value for verifying its progression (ascending/descending), and the await_all_final_values_reported function doesn't return anything
expected_final_value = [
AttributeValue(
endpoint,
attribute=Clusters.FanControl.Attributes.FanMode,
value=???????
)]
self.attribute_subscription.await_all_final_values_reported(
expected_final_value,
timeout_sec=1)
So:
- Expected value? Unknown
- When to expect it? Unknown
I consider I need to keep the code as is, unless I'm missing something.
There was a problem hiding this comment.
OK, I see what you're saying, but you're racing the flush here, which is why I'm a bit concerned.
What about if you used multiple subscriptions - one for the attribute being updated, so you can see it going in real-time, one for the whole cluster? In each iteration of the for loop, await the one being updated on the single attribute subscription, then at the end of the cycle, await the final sequence of all the expeced attribute reports. Then you can look back in the queue and make sure everything is normal - all modes separated by at least one percent updadte, modes in order, modes all there etc.
thoughts?
src/python_testing/TC_FAN_3_1.py
Outdated
| await self.get_initialization_parametes(attr_to_update, order) | ||
|
|
||
| # Initializatization of the attribute to update (Write and read back verification) | ||
| await self.write_and_verify_attribute(endpoint, attr_to_update, value_init_update) |
There was a problem hiding this comment.
If you get INVALID_IN_STATE here, what happens?
src/python_testing/TC_FAN_3_1.py
Outdated
| await self.write_and_verify_attribute(endpoint, attr_to_update, value_init_update) | ||
|
|
||
| # Initializatization of the attribute to verify (Write and read back verification) | ||
| attr_value_read = await self.write_and_verify_attribute(endpoint, attr_to_verify, value_init_verify) |
There was a problem hiding this comment.
If you just wrote the update value, why are you writing the verification value? They should update together.
| # Get current attribute value and verify correct increment/decrement | ||
| queue = self.attribute_subscription.attribute_queue.queue | ||
| attr_value_current = await self.get_attribute_value_from_queue(queue, attr_to_verify, timeout_sec) | ||
| if attr_value_current is not None: |
There was a problem hiding this comment.
This should never be None.
| speed_setting_previous = speed_setting_read | ||
| for value_to_write in iteration_range: | ||
| # Clear the attribute report queue before each update to avoid duplicates | ||
| self.attribute_subscription.get_last_report() |
There was a problem hiding this comment.
Suggested change
| self.attribute_subscription.get_last_report() | |
| self.attribute_subscription.flush_reports() |
There was a problem hiding this comment.
Actually, though, do you want to do this? Say, for example, you're updating the percent. If the percent attribute updates first, the loop will complete and you'll be back to the top here. You're then racing with the mode update to see whether it lands before you flush, in which case it's lost, or whether it appears in the next loop iteration. I think what you probably want to do instead is to just let the dependent attribute changes for mode slop between the percent settings, then wait specifically for the ones you KNOW need to happen.
| attr_value_previous = attr_value_read | ||
| speed_setting_current = speed_setting_read | ||
| speed_setting_previous = speed_setting_read | ||
| for value_to_write in iteration_range: |
There was a problem hiding this comment.
I see what you're trying to do here, but I think the conditions here might be sufficiently different that your code is longer and more confusing by trying to make these the same and pipe them through a single function
for percent, you want:
- update percent
- check for percent update every time (report + read)
- if mode change happened, check that the mode is incrementing or decrementing as expected (report + read)
- ditto for speed
- at the end ensure the last mode change happened
- ditto for speed
- for both mode and speed, you want to make sure you saw all the possibilities (except auto) and you want to make sure you didn't see auto.
for update mode you want:
- update mode
- check that the mode change happened (report and read)
- check that the percent change happened (every time) and is going in the correct direction (report and read)
- speed change should happen every time IF the number of speed settings is larger than the number of modes
There's commonalities, but it might make more sense to break this as
- update attribute, wait for report (with INVALID_IN_STATE handing) as a single function
- if INVALID_IN_STATE, verify no changes
- if SUCCESS verify dependent attributes (reports and reads, with a parameter saying if the change is required)
src/python_testing/TC_FAN_3_1.py
Outdated
| # Initializatization of the SpeedSetting attribute, if supported (Write and read back verification) | ||
| if self.supports_speed: | ||
| speed_setting_init = 0 if order == OrderEnum.Ascending else self.speed_max | ||
| speed_setting_read = await self.write_and_verify_attribute(endpoint, speed_setting_attr, speed_setting_init) |
There was a problem hiding this comment.
same thing here - speed should update itself when the initial attribute is updated.
…ranges to inclue 0
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Testing
Addresses
[TC-FAN] Fan tests do not sufficiently test attribute interactions #4788
Testing criteria
The main testing being performed is verifying that the values for PercentSetting, FanMode, or SpeedSetting (if supported) are being updated correctly. Specifically, it checks whether updating PercentSetting or FanMode, which triggers a change in the other, results in the current value being greater than or less than the previous value (depending on whether the test is performed in ascending or descending order, respectively). This check is done when the status code for the update is SUCCESS. Alternatively, when the status code is INVALID_IN_STATE, the test verifies that the current value is the same as the previous value.
This enhancement also requires a test plan update