[XL] Change in dart/sdk[main]: DeCo. Refactor index tests.
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 1 comment![]( "Open in Gerrit")
Konstantin ShcheglovI don't feel that I can review this CL effectively because it combines so many renames with the addition of so many new tests. This means that in order to review it I will have to (a) skip review of the test file (which seems irresponsible), (b) track down which tests are new and which tests are old by doing a lot of searching, or (c) just go ahead and review all the tests.
What would be ideal for me is if you could split this into two CLs: one that renames the existing tests, and a follow-up CL that adds the new tests. Then, I could reassure myself that the first CL only contained renames by seeing that the diff contained an equal number of added and removed lines, and so I could focus my review effort on the second CL.
Paul BerryUnfortunately this is not just rename, but also re-group test situations, in a way described in the CL message. I believe the easiest approach would be just review all the tests.
Konstantin ShcheglovOk, I didn't realize it was regrouping as well as renaming. Would it be possible to have a CL that renames and regroups the existing tests, followed by one that adds the new tests?
Judging by the size of the diffs in index_test.dart (-2342, +3264), I'm guessing that there's about 2342 lines of tests that were regrouped and renamed and about 3264-2342=922 lines of tests that were added, so if the CL were split into two in the way I suggest, it would be about 2342 lines of renaming and regrouping (which I would feel comfortable giving a much less thorough review), followed by 922 lines of added tests, which I would review more thoroughly. If we can find an approach where I only have to review 922 lines thoroughly rather than 3264, I believe that would be a far more efficient use of the time I have available for code review.
Paul BerryI believe that there is already segregation in place. New tests are around primary constructors. Look for `_primary` and `_newHead` categories in the test method names.
Konstantin ShcheglovGlad to hear there is already segregation in place. I believe that means that it would be relatively easy for you to create the two CLs I suggested. Am I understanding that correctly?
OK, I re-purposed this CL to re-group tests.
Related details
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Berry (Gerrit)
Paul Berry voted and added 4 comments![]( "Open in Gerrit")
Votes added by Paul Berry
| Code-Review | +1 |
Konstantin ShcheglovI don't feel that I can review this CL effectively because it combines so many renames with the addition of so many new tests. This means that in order to review it I will have to (a) skip review of the test file (which seems irresponsible), (b) track down which tests are new and which tests are old by doing a lot of searching, or (c) just go ahead and review all the tests.
What would be ideal for me is if you could split this into two CLs: one that renames the existing tests, and a follow-up CL that adds the new tests. Then, I could reassure myself that the first CL only contained renames by seeing that the diff contained an equal number of added and removed lines, and so I could focus my review effort on the second CL.
Paul BerryUnfortunately this is not just rename, but also re-group test situations, in a way described in the CL message. I believe the easiest approach would be just review all the tests.
Konstantin ShcheglovOk, I didn't realize it was regrouping as well as renaming. Would it be possible to have a CL that renames and regroups the existing tests, followed by one that adds the new tests?
Judging by the size of the diffs in index_test.dart (-2342, +3264), I'm guessing that there's about 2342 lines of tests that were regrouped and renamed and about 3264-2342=922 lines of tests that were added, so if the CL were split into two in the way I suggest, it would be about 2342 lines of renaming and regrouping (which I would feel comfortable giving a much less thorough review), followed by 922 lines of added tests, which I would review more thoroughly. If we can find an approach where I only have to review 922 lines thoroughly rather than 3264, I believe that would be a far more efficient use of the time I have available for code review.
Paul BerryI believe that there is already segregation in place. New tests are around primary constructors. Look for `_primary` and `_newHead` categories in the test method names.
Konstantin ShcheglovGlad to hear there is already segregation in place. I believe that means that it would be relatively easy for you to create the two CLs I suggested. Am I understanding that correctly?
OK, I re-purposed this CL to re-group tests.
Thanks!
44 5:7 |B| IS_ANCESTOR_OFSomething you might want to consider for a future CL:
This reads to me as "B is the ancestor of <something>", whereas actually I believe the relation that's being asserted here is that "A (the thing that we passed to findElement2) is an ancestor of B". Maybe in the future it would be useful to change the output format of _getRelationsText so that this relation is shown like "IS_ANCESTOR_OF |B|".
54 5:17 |A| IS_EXTENDED_BYSimilarly, I think this would read more straightforwardly as "IS_EXTENDED_BY |A|", since the relation it's asserting is "A (the thing that we passed to findElement2) is extended via the text "A" that appears here".
Related details
- Johnni Winther
- Konstantin Shcheglov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Konstantin Shcheglov
| Commit-Queue | +2 |
1 comment
54 5:17 |A| IS_EXTENDED_BYSimilarly, I think this would read more straightforwardly as "IS_EXTENDED_BY |A|", since the relation it's asserting is "A (the thing that we passed to findElement2) is extended via the text "A" that appears here".
I agree that `|B| IS_ANCESTOR_OF` reads not quite right, not as the actual relation it represents.
There are a few other constraints that I had in mind. First, put the text like `|B|` next to the location, because this text serves as the "length" of the location, with an additional bonus that it shows also the selected text.
Second, `54 5:17 |A| IS_EXTENDED_BY` actually reads right, here the order is correct. The element that corresponds to the location, `class B`, extends `A`.
Similarly `54 5:17 |A| IS_REFERENCED_BY`, I think reads right.
And so, for consistency I think it is better to keep the format as is, and don't move the selected text depending on the relation kind.
OTOH, we might not need `IS_ANCESTOR_OF` at all, a quick check did not show that we use it, but I will try to remove and do more analysis separately.
Related details
- Johnni Winther
- Paul Berry
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Commit Queue (Gerrit)
Commit Queue submitted the change![]( "Open in Gerrit")
Change information
DeCo. Refactor index tests.
Rework analyzer index tests to be more maintainable and to better match
the DeCo/primary-constructor surface area.
Test methods are named in general as
`test_<ElementClass>_<container>_<details>` or
`test_<ElementClass>_<usageGroup>_<details>`. This makes it easier to
see for which elements we have tests, and provide better coverage by
freely adding usages to the same code.
Improve failure diagnostics by printing a structured diff when expected
index text mismatches.
- M pkg/analyzer/lib/src/test_utilities/find_element2.dart
- M pkg/analyzer/test/src/dart/analysis/index_test.dart
Code-Review: +1 by Paul Berry
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |