2nd attempt to fix random EEM and EPM CI failures #34476
Merged
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
…ng fake readings have changed." This reverts commit 42d36e9.
…different values between two readings. There is a high probability that 2 energy readings could be the same, so using a fixed seed should mean that tests are at least reproducible.
pullapprove
bot
requested review from
andy31415,
andyg-apple,
anush-apple,
arkq,
axelnxp,
bauerschwan,
bzbarsky-apple,
carol-apple and
chapongatien
pullapprove
bot
requested review from
chrisdecenzo,
chshu,
chulspro,
cliffamzn,
Damian-Nordic,
dhrishi,
doru91,
fessehaeve,
harimau-qirex,
harsha-rajendran,
hawk248 and
hicklin
pullapprove
bot
requested review from
robszewczyk,
saurabhst,
selissia,
sharadb-amazon,
tcarmelveilleux,
tecimovic,
tehampson,
tima-q,
tobiasgraf,
turon,
vivien-apple,
wiba-nordic,
woody-apple,
younghak-hwang and
yufengwangca
|
PR #34476: Size comparison from 8306353 to 117b708 Full report (82 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nxp, psoc6, qpg, stm32, telink, tizen)
|
andy31415
approved these changes
Jul 24, 2024
bzbarsky-apple
approved these changes
Jul 26, 2024
|
@jamesharrow could you fix merge conflicts? |
…try-to-avoid-CI-failures
…gs.cpp after updating to master.
|
PR #34476: Size comparison from f4d9188 to 56b398a Full report (85 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, mbed, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
jamesharrow
deleted the
EEM-/-EPM-add-fix-random-seed-to-try-to-avoid-CI-failures
branch
j-ororke
pushed a commit
to j-ororke/connectedhomeip
that referenced
this pull request
Jul 31, 2024
* Revert "Attempt to reduce CI failures by increasing timeout in checking fake readings have changed." This reverts commit 42d36e9. * Added fixed random seed to try to avoid CI failures when looking for different values between two readings. There is a high probability that 2 energy readings could be the same, so using a fixed seed should mean that tests are at least reproducible. * Restyled by whitespace * Improved comment * Moved the change previously made in EVSManufacturerImpl to FakeReadings.cpp after updating to master. * Restyled by whitespace --------- Co-authored-by: Restyled.io <[email protected]>
rochaferraz
pushed a commit
to rochaferraz/connectedhomeip
that referenced
this pull request
Jul 31, 2024
* Revert "Attempt to reduce CI failures by increasing timeout in checking fake readings have changed." This reverts commit 42d36e9. * Added fixed random seed to try to avoid CI failures when looking for different values between two readings. There is a high probability that 2 energy readings could be the same, so using a fixed seed should mean that tests are at least reproducible. * Restyled by whitespace * Improved comment * Moved the change previously made in EVSManufacturerImpl to FakeReadings.cpp after updating to master. * Restyled by whitespace --------- Co-authored-by: Restyled.io <[email protected]>
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.
After more review of the troublesome failures - it looks like the failures tend to be in the EEM cluster when checking that 2 energy readings are different values.
The reason for this is because we compute a random "power" in mW - e.g. -3000000 +/- 20000mW (i.e. somewhere between -2980W and -3020W.
But then we convert this into energy over 5 seconds in mWh by x5 / 3600s = so our values become in the range 4138 to 4194.
Or in the case of the Load - we generate random power values in the range 1000000 +/- 20000mW every 2 seconds.
So the energy values are only in the range 544 to 566.
So sooner or later we will get 2 consecutive values which are the same (even if we skipped a few values in between).
So adding a fixed random seed should help this when running under CI.
Since the previous attempt at introducing more delay hasn't helped, I have reverted the change to the test scripts to remove these delays, and this means that the test plans do not need to change - so issue https://github.com/CHIP-Specifications/chip-test-plans/issues/4284 can be closed.