Re-Fix build being called before element is ready to build. #4476
Conversation
mkhatib
force-pushed
the
fix-build-ready-bug
branch
from
dca6e50 to
bf89cdc
Compare
| @@ -21,8 +21,6 @@ function checkElementUpgrade(element) { | |||
| expect(element).to.have.class('-amp-element'); | |||
| expect(element).to.have.class('-amp-layout-responsive'); | |||
| expect(element).to.have.class('-amp-layout-size-defined'); | |||
| expect(element).to.not.have.class('-amp-notbuilt'); | |||
There was a problem hiding this comment.
Yes sorry. So @zhouyx here is testing only if the element was upgraded and so checking if it doesn't have -amp-unresolved should be enough. Our theory is that the event the test is waiting on is fired maybe too early before the iframe gets built and we weren't able to wait on amp:load:start to fire for the iframe since it might not be laid out because it probably is out of the viewport during the test.
So we opted for just dropping testing the build part in this test and just test upgrade which what the test is intended for.
There was a problem hiding this comment.
so removing this works @mkhatib ?
Yes since after the change we don't build directly after upgrade, we can't expect element to be built here. if we dispatch event like amp:built later, we can introduce integration test that test all element gets built correctly. (If necessary)
There was a problem hiding this comment.
Yes it passed when I made this change.
@dvoytenko pointed out that we might be breaking something here so I am doing few other runs with different setup, the safest path he suggested is to bring back the build during discoverWork_ but protect against unready-builds. Testing few combinations now will report back.
There was a problem hiding this comment.
Ok. brought back the build during discover - and changed the tests to wait on multiple events for the elements numbers. This has passed but the test flaked - rerunning the test for this but it should be good to go.
|
LGTM. defer to @dvoytenko |
|
LGTM |
This is a resubmit of a reverted PR with a fix to the broken test
Original PR: #4426
Revert PR: #4467
Fixes #4397