-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add unit tests for the layout engine #424
Conversation
2efb7b6
to
c894eb3
Compare
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.
This is incredibly cool! We need more tests like this before fixing any existing problems in the layout system. Thank you for taking the first stab!
Regarding the orphan-node issue, I think you can reproduce by first start a layout transition with animation, then while the animation is running, trigger another layout pass on the root node via -setNeedsLayout
. Depending on the differences between the original layout and the first transition's layout, expect both orphaned subnode(s) as well as subnode(s) that should be inserted but never was. More information can be found in facebookarchive/AsyncDisplayKit#2862.
Following the same steps above, but instead of triggering the second/last layout pass via -setNeedsLayout
, trigger another async layout transition. Even if the second transition is queued up correctly and starts applying its result on main after the animation of the first transition finished, what will happen is that the second transition will run the ASM diff-ing algorithm against the original layout, instead of the first transition's layout. That is because layout transition contexts capture the from/original layout at initialization time, as opposed to at commit time. And that can also cause orphaned or never-inserted subnodes.
Tests/ASLayoutEngineTests.mm
Outdated
fixture.layout = layoutA; | ||
|
||
ASLayoutSpecBlock specBlockA = ^ASLayoutSpec * _Nonnull(__kindof ASDisplayNode * _Nonnull node, ASSizeRange constrainedSize) { | ||
return [ASStackLayoutSpec stackLayoutSpecWithDirection:ASStackLayoutDirectionHorizontal spacing:0 justifyContent:ASStackLayoutJustifyContentSpaceBetween alignItems:ASStackLayoutAlignItemsStart children:@[ nodeB, nodeC, nodeE ]]; |
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.
Shouldn't the children
contains nodes B, D, E instead of B, C, E?
Tests/ASLayoutEngineTests.mm
Outdated
* From fixture 2: | ||
* B survives with same layout | ||
* C is removed | ||
* D is removed |
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.
Should be "D joins the first layout" since it's a new subnode.
// The code in this section is designed to move in time order, all on the main thread: | ||
|
||
OCMExpect([nodeA.mock animateLayoutTransition:OCMOCK_ANY]).onMainThread(); | ||
OCMExpect([nodeA.mock didCompleteLayoutTransition:OCMOCK_ANY]).onMainThread(); |
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.
These method calls are only expected when the correct behavior is not enforced. The correct behavior is that the layout transition will be cancelled and this it won't animate and complete.
* While it's measuring, on main switch to fixture 4 (setNeedsLayout A, D) and run a CA layout pass. | ||
* | ||
* Correct behavior, we end up at fixture 4 since it's newer. | ||
* Current incorrect behavior, we end up at fixture 2 and we remeasure surviving node C. |
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 believe this unexpected behavior is caused by this early check in __layout
, which is introduced in #2657.
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// |
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.
Do we really need this license? Or added just to make Danger happy?
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 to make Danger happy.
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.
That's sad :(
* The returned value (8) is clamped to the fixed with (7), and then compared to the previous | ||
* width (7) and we decide not to propagate up the invalidation, and we stay stuck on the old | ||
* layout (fixture3). | ||
*/ |
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.
Is this because nodes only store their last-used constraint?
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.
Yes exactly!
Btw, I don't think this diff is trivial. I'd suggest to add something to the change log. It's your call though. |
🚫 CI failed with log |
The CI failure this time was interesting! I'll put this info somewhere else in the future, but I want to make sure not to lose it: This is an extremely idealized version of a 3 hour slog to diagnose a very tricky (for me) race condition. I believe next time the slog can be way shorter with these learnings. To diagnose, I override
Note: I also had to change the I then turned on verbose logging Then I set a breakpoint where Then I walked away and got a coffee while the 10000 iterations ran. When I came back, I was at the failure breakpoint (the extra layout pass wasn't running). So I dumped the log for that iteration, plus the previous iteration. I use Sublime text to strip out all timestamps and pointer values from the log, then I ran I added some more log statements to cover when _pendingDisplayNodeLayout changed and ran the process again. I grabbed the good & bad log chunks, stripped them down, ran the diff and got this diff: Conclusion: If CA runs a layout pass on the main thread before the transition can get a hold of nodeC's lock on the background thread, then no extra layout happens because the pending layout that gets established can be used in the layout pass for the new fixture (C's layout between the two target layouts is the same, D's is not and that's why its behavior is more predictable). If instead CA runs its layout pass after the bg layout gets set as pending, nodeC will discard it as part of the layout pass and have to regenerate it. Note that it's really nodeA's lock that matters here, since nodeA's lock is held throughout the calculation process, and nodeA needs layout so it's a race for the two threads to lock node A. Not sure yet what I'll do with this knowledge, but there it is. |
@Adlai-Holler Please fix the file conflicts and merge away. |
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.
Let's get that in!
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
🚫 CI failed with log |
9552195
to
5d38b33
Compare
🚫 CI failed with 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.
Thanks for picking this up man. Empty test failure log. wth? cc @garrettmoon any ideas here? It took an hour which suggests a timeout?
Source/Layout/ASLayout.mm
Outdated
} | ||
return NO; | ||
} | ||
|
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.
Oh wow. Very very scary that this never warned of duplicate method implementation.
FYI, these new tests pass locally if I apply this diff on top of #695. And yes, we should make sure failure logs have enough info. |
Before: - Even if a pending layout was applied before, it'll be unnecessarily applied again in next layout passes and cause `-calculatedLayoutDidChange` being called multiple times. After: - If a pending layout was applied, the calculated layout will not be ignored but reused, if possible, in next layout passes. Test plan: testSetNeedsLayoutAndNormalLayoutPass in #424.
5d38b33
to
ceddc77
Compare
🚫 CI failed with log |
Generated by 🚫 Danger |
…reGroup#695) Before: - Even if a pending layout was applied before, it'll be unnecessarily applied again in next layout passes and cause `-calculatedLayoutDidChange` being called multiple times. After: - If a pending layout was applied, the calculated layout will not be ignored but reused, if possible, in next layout passes. Test plan: testSetNeedsLayoutAndNormalLayoutPass in TextureGroup#424.
* Build testing platform & tests for the layout engine * Add our license header to debugbreak. * Remove thing * Address review comments * Beef up the logging * Update -[ASLayout isEqual:] * testLayoutTransitionWithAsyncMeasurement passes now * Disable testASetNeedsLayoutInterferingWithTheCurrentTransition * Fix build errors
This adds a testing platform and some unit tests to the layout engine. Right now there are flags to toggle between the current behavior and the desired behavior, and they're set to "current behavior."
Issues reproduced (need to merge with GitHub issues):
calculatedLayoutDidChange
calls. Not a biggie.setNeedsLayout
and get a layout pass while a transition is measuring, the transition's "to-layout" will win, but the setNeedsLayout+layoutPass should actually win since its data is newer. https://github.com/TextureGroup/Texture/pull/424/files#diff-b1441b7e61c34b6f132da6c94bb10c0cR204I'd like to reproduce the orphan-node issue but I'm not sure what conditions would trigger it @nguyenhuy. We've got enough flexibility in the testing platform – I believe – to test an incredible variety of different scenarios.
PROTIP: You can take any expectation and add
.andDebugBreak()
to get a breakpoint when that call happens. So if you expect an async layout:OCMExpect([nodeA.mock calculateLayoutThatFits:]).offMainThread()
you can change that toOCMExpect([nodeA.mock calculateLayoutThatFits:]).offMainThread().andDebugBreak()
and when that calculateLayoutThatFits invocation happens, the debugger will pause for you. ✨Magic✨