[XL] Change in dart/sdk[main]: DeCo. Refactor index tests, add primary constructor tests.
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov added 2 comments![]( "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.
Unfortunately 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.
DeCo. Refactory index tests, add primary constructor tests.Konstantin ShcheglovChange to `Refactor`.
Done
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 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.
Unfortunately 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.
Ok, 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.
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 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.
Ok, 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.
I 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.
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 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.
I 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.
Glad 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?
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. |