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

Delay processing test event in Smoke CO Alarm #28044

Merged
merged 1 commit into from
Jul 19, 2023

Conversation

ericzijian1994
Copy link
Contributor

Because TH run step is slow, delay the processing of test event.

@mergify mergify bot merged commit 435aa96 into project-chip:master Jul 19, 2023
@bzbarsky-apple
Copy link
Contributor

Hold on. This seems to be working around some bug in the test plan, but are all the actual devices supposed to work around this bug too? @cjandhyala

@cjandhyala
Copy link
Contributor

not sure why we need that delay to triggerEvent. @ericzijian1994 any TH ticket that you filed ?

@ericzijian1994 ericzijian1994 deleted the fix-testEventTrigger branch July 20, 2023 06:44
@hare-siterwell
Copy link
Contributor

not sure why we need that delay to triggerEvent. @ericzijian1994 any TH ticket that you filed ?

Because TH is a slow test, it will wait a few seconds after sending the eventTrigger command before executing the waitForReport step. This would miss the timing of the attribute change, causing waitForReport to wait until it times out. So I added the timer to change the attribute 5 seconds after receiving the eventTrigger command.

@ericzijian1994
Copy link
Contributor Author

I don't know if this is a bug, maybe waitForReport needs to be optimized. @cjandhyala

@bzbarsky-apple
Copy link
Contributor

If waitForReport only sees reports that come after it starts, that's a problem. There are tons of tests that assume they can change a thing and then waitForReport.... @vivien-apple @cjandhyala

@bzbarsky-apple
Copy link
Contributor

And in particular: we can't expect real devices to delay processing like this! But we do want them to pass our cert tests. So if our cert tests are buggy we need to fix those bugs, not just work around them.

erwinpan1 pushed a commit to erwinpan1/connectedhomeip that referenced this pull request Jul 21, 2023
@cjandhyala
Copy link
Contributor

I totally agree we can not add delay in the SDK code to work around a TH issue. If you see any issue with TH, pls file ticket on TH.

@hare-siterwell
Copy link
Contributor

Looks like #19861 has already mentioned this. @cjandhyala

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

Successfully merging this pull request may close these issues.

5 participants