[L] Change in dart/sdk[main]: CQ. Use assertDartObjectText() with raw multi-line string in more pla...
0 views
Skip to first unread message
Paul Berry (Gerrit)
Jun 2, 2026, 4:23:50 PM (yesterday) Jun 2
to Konstantin Shcheglov, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Johnni Winther and Konstantin Shcheglov
Paul Berry voted and added 3 comments![]( "Open in Gerrit")
Votes added by Paul Berry
| Code-Review | +1 |
3 comments
Patchset-level comments
File pkg/analyzer/test/src/dart/constant/evaluation_test.dart
Line 4249, Patchset 2 (Latest):
var resultB = _evaluateConstant(result, 'b');Paul Berry . unresolved
We've lost the assertion that A and B evaluate to the same value. In fact, we're not even checking what A evaluates to anymore. That seems like a regression.
At a minimum I'd prefer for the test to check the value of A as well a B.
Ideally, keep the line `expect(resultB, resultA);` as well. This would have the advantage that if a change were accidentally made that caused the two results to be different, the mistake would be caught, even if test expectations were automatically updated.
Line 4269, Patchset 2 (Latest):
var resultB = _evaluateConstant(result, 'b');Paul Berry . unresolved
Same issue here.
Related details
Attention is currently required from:
- Johnni Winther
- Konstantin Shcheglov
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Jun 2, 2026, 7:52:53 PM (21 hours ago) Jun 2
to Paul Berry, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Johnni Winther
Konstantin Shcheglov added 2 comments![]( "Open in Gerrit")
File pkg/analyzer/test/src/dart/constant/evaluation_test.dart
We've lost the assertion that A and B evaluate to the same value. In fact, we're not even checking what A evaluates to anymore. That seems like a regression.
At a minimum I'd prefer for the test to check the value of A as well a B.
Ideally, keep the line `expect(resultB, resultA);` as well. This would have the advantage that if a change were accidentally made that caused the two results to be different, the mistake would be caught, even if test expectations were automatically updated.
Konstantin Shcheglov
Done
Line 4269, Patchset 2:
var resultB = _evaluateConstant(result, 'b');Paul Berry . resolved
Konstantin ShcheglovSame issue here.
Done
Related details
Attention is currently required from:
- Johnni Winther
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Jun 2, 2026, 8:03:33 PM (21 hours ago) Jun 2
to Paul Berry, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Johnni Winther
Konstantin Shcheglov voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Attention is currently required from:
- Johnni Winther
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Paul Berry (Gerrit)
12:27 PM (5 hours ago) 12:27 PM
to Konstantin Shcheglov, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, dart-analys...@google.com, rev...@dartlang.org
Attention needed from Johnni Winther and Konstantin Shcheglov
Paul Berry voted and added 1 comment![]( "Open in Gerrit")
Votes added by Paul Berry
| Code-Review | +1 |
Attention is currently required from:
- Johnni Winther
- Konstantin Shcheglov
Submit Requirements:
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
2:14 PM (3 hours ago) 2:14 PM
to Paul Berry, Johnni Winther, dart-...@luci-project-accounts.iam.gserviceaccount.com, dart-analys...@google.com, rev...@dartlang.org
Konstantin Shcheglov submitted the change![]( "Open in Gerrit")
Change information
Commit message:
CQ. Use assertDartObjectText() with raw multi-line string in more places.
Replace ad hoc assertions over DartObjectImpl with
assertDartObjectText() in constant evaluation and resolution tests. This
makes the expected constant value shape explicit, including invalid
results, variables, constructor invocations, type arguments, and
superclass fields, instead of checking only selected fields.
Use raw multi-line strings for these expectations so expected text can
be copied and maintained consistently without escaping interpolation or
other Dart string syntax.
Remove the now-unused null assertion helper and analyzer implementation
imports that were only needed by the direct assertions.
Change-Id: Ib734c1d428dd69104403484534b60af918944d13
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/508705
Reviewed-by: Paul Berry <paul...@google.com>
Files:
- M pkg/analyzer/test/src/dart/constant/evaluation_test.dart
- M pkg/analyzer/test/src/dart/resolution/constant_test.dart
Change size: L
Delta: 2 files changed, 403 insertions(+), 247 deletions(-)
Branch: refs/heads/main
Submit Requirements:
Code-Review: +1 by Paul Berry
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages