-
Notifications
You must be signed in to change notification settings - Fork 28
#1401. [Patterns] Logical and relational patterns tests refactored #1573
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did I forget all about how logging works? As far as I can see, there are a number of situations where the log is expected to contain evidence for several comparisons, but I would expect only the first invocation of each getter on any specific object to be visible, because subsequent comparisons will use the cached value of that getter invocation.
I suggested a number of changes based on this understanding of the code.
break; | ||
default: | ||
} | ||
Expect.equals("Square.area=1;Square.area=2;", shape2.log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmmm, does this imply that shape2.area
is invoked twice? It should be cached when it is compared with something (here 1
), and the comparison to other things (here 2
) should then rely on the cached value rather than calling area
again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Thank you for noticing! shape2.area
getter returns Unit
object that will be cached. But this object has logger enabled, so at each comparison it will call _log("=$other;");
. So here and in other places we should expect
Expect.equals("Square.area=1;=2;", shape2.log);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these things are working now. Resolving a bunch of threads.
@eernstg I fixed the tests according your notes. Please rereview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid we still need to clarify the semantics of some matching steps: I don't think it is possible to observe some of the tests, in spite of the fact that the log is expected to reveal them. Details given in a comment on one such location.
@eernstg logging is fixed. Please review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one location looks like there are too many elements in the log.
|
||
void logger(String toAppend) { | ||
_log = _log.isEmpty ? toAppend : "$_log$toAppend"; | ||
void _log(String toLog) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this could be:
void _log(String toLog) { | |
void _log(String toLog) => logFunction?.call(toLog); |
break; | ||
default: | ||
} | ||
Expect.equals("Square.area=1;Square.area=2;", shape2.log); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these things are working now. Resolving a bunch of threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
2022-12-09 [email protected] dart-lang/co19#1401. [Patterns] List's rest pattern tests (dart-lang/co19#1578) 2022-12-08 [email protected] dart-lang/co19#1575. Update test expectations for analyzer (dart-lang/co19#1576) 2022-12-07 [email protected] dart-lang/co19#1401. [Patterns] Map patterns tests updated. Check primitive == for keys (dart-lang/co19#1574) 2022-12-07 [email protected] dart-lang/co19#1401. [Patterns] Logical and relational patterns tests refactored (dart-lang/co19#1573) 2022-12-06 [email protected] dart-lang/co19#1401. [Patterns] Map pattern tests (dart-lang/co19#1572) 2022-12-02 [email protected] Fixes dart-lang/co19#1553. Description wording fixed (dart-lang/co19#1569) 2022-12-02 [email protected] Fixes dart-lang/co19#1568. Exclude VM-specific test form web-platforms (dart-lang/co19#1570) Change-Id: I69e3d49ac0b244ed12c266bd00a1c3ec9f8e1b45 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/274660 Reviewed-by: Alexander Thomas <[email protected]>
if-case
statementscase a && b || c && d