Fix YAML tests handling of lists of structs that contain lists. #13142
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
bzbarsky-apple
force-pushed
the
lists-inside-lists
branch
from
49c1da5 to
4138f0e
Compare
pullapprove
bot
requested review from
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
carol-apple,
cecille,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
hawk248,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kghost and
kpschoedel
pullapprove
bot
requested review from
mlepage-google,
msandstedt,
pan-apple,
robszewczyk,
sagar-apple,
saurabhst,
selissia,
tcarmelveilleux,
tecimovic,
vijs,
wbschiller,
woody-apple and
xylophone21
|
PR #13142: Size comparison from d6aac1f to 4138f0e Increases above 0.2%:
Increases (5 builds for esp32, linux, mbed, p6)
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
andy31415
approved these changes
Jan 6, 2022
bzbarsky-apple
force-pushed
the
lists-inside-lists
branch
from
4138f0e to
50595f2
Compare
|
PR #13142: Size comparison from 46541fb to 50595f2 Increases above 0.2%:
Increases (2 builds for linux, p6)
Full report (14 builds for efr32, k32w, linux, p6, qpg, telink)
|
The main issue was that comparing to such a value failed because we ended up with "iter" variables for both levels of list nesting that name-collided. The fix for that was to add "depth" disabmbiguation like we have in the Darwin codegen already. While adding a test for this, a few other issues were encountered: 1) The "commandValue" partial, when dealing with an optional or nullable struct, would do an Emplace() or SetNonNull() for every single member, which would wipe out earlier members. The fix for that is to do them once, and then use .Value() for the recursive partial invocation. 2) When sending a struct with optional fields, we were requiring the YAML to specify values for all the fields. Added if_include_struct_item_value to allow the YAML to not include values for such fields. 3) The YAML for checking struct values expected struct field names in the YAML to be lowerCamelCase instead of matching the XML case. The code for sending struct values expected struct field names to match the XML case. This inconsistency was very confusing, and this PR makes both expect the XML case. I verified that the only codegen changes due to this were in the TestModeSelectCluster and Test_TC_DM_2_2 tests. The latter had actually had its struct fields ignored because they were the "wrong" case.... Both tests fixed.
bzbarsky-apple
force-pushed
the
lists-inside-lists
branch
from
50595f2 to
b1c5973
Compare
|
PR #13142: Size comparison from 89b87f1 to b1c5973 Increases above 0.2%:
Increases (4 builds for esp32, linux, p6)
Full report (26 builds for efr32, esp32, k32w, linux, nrfconnect, p6, qpg, telink)
|
step0035
pushed a commit
to hank820/connectedhomeip
that referenced
this pull request
Feb 8, 2022
…ect-chip#13142) The main issue was that comparing to such a value failed because we ended up with "iter" variables for both levels of list nesting that name-collided. The fix for that was to add "depth" disabmbiguation like we have in the Darwin codegen already. While adding a test for this, a few other issues were encountered: 1) The "commandValue" partial, when dealing with an optional or nullable struct, would do an Emplace() or SetNonNull() for every single member, which would wipe out earlier members. The fix for that is to do them once, and then use .Value() for the recursive partial invocation. 2) When sending a struct with optional fields, we were requiring the YAML to specify values for all the fields. Added if_include_struct_item_value to allow the YAML to not include values for such fields. 3) The YAML for checking struct values expected struct field names in the YAML to be lowerCamelCase instead of matching the XML case. The code for sending struct values expected struct field names to match the XML case. This inconsistency was very confusing, and this PR makes both expect the XML case. I verified that the only codegen changes due to this were in the TestModeSelectCluster and Test_TC_DM_2_2 tests. The latter had actually had its struct fields ignored because they were the "wrong" case.... Both tests fixed.
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.
The main issue was that comparing to such a value failed because we
ended up with "iter" variables for both levels of list nesting that
name-collided.
The fix for that was to add "depth" disabmbiguation like we have in
the Darwin codegen already.
While adding a test for this, a few other issues were encountered:
The "commandValue" partial, when dealing with an optional or
nullable struct, would do an Emplace() or SetNonNull() for every
single member, which would wipe out earlier members. The fix for that
is to do them once, and then use .Value() for the recursive partial
invocation.
When sending a struct with optional fields, we were requiring the
YAML to specify values for all the fields. Added
if_include_struct_item_value to allow the YAML to not include values
for such fields.
The YAML for checking struct values expected struct field names in
the YAML to be lowerCamelCase instead of matching the XML case. The
code for sending struct values expected struct field names to match
the XML case. This inconsistency was very confusing, and this PR
makes both expect the XML case. I verified that the only codegen
changes due to this were in the TestModeSelectCluster and
Test_TC_DM_2_2 tests. The latter had actually had its struct fields
ignored because they were the "wrong" case.... Both tests fixed.
Problem
See above.
Change overview
See above.
Testing
New tests pass. Manually examined codegen changes for tests for sanity.