amp-list: Hide placeholder et al. after re-render with local data by dreamofabear · Pull Request #17209 · ampproject/amphtml

dreamofabear · 2018-07-31T04:12:55Z

This was caused by #16843. Previously, the placeholder was only shown and hidden in fetchList_(). The PR moved the "showing" to all re-render triggers, but did not copy the "hiding" to corresponding places. This causes the placeholder/fallback/loading to fail to be hidden when re-rendering with local data.

Other things I noticed that I'd like to fix:

Figure out why two tests caused all following tests to be silently skipped
Refactor resetIfNecessary_, hideFallbackAndPlaceholder_, showFallback_ into a single function that takes a tri-state enum: FETCHING, SUCCESS, FAIL

/to @aghassemi

dreamofabear · 2018-07-31T04:16:59Z

    });

-    it('should fail to load b/c data array is absent', () => {
+    // TODO: This test passes but causes all following tests to be ignored.


@rsimha Are async throws still an issue? We may be silently skipping other tests too.

@choumx Seems like there's an uncaught error originating from this test. I don't know if this is an async throw (rethrowAsync is no longer asynchronous during tests). I suspect it's a case of #15658 being triggered.

Note: This sort of silent skipping should only happen during local development, where we surface unhandled errors.

Edit: I'm working on a more general fix.

amp-list doesn't use rethrowAsync but it does throw errors asynchronously in some cases.

dreamofabear · 2018-07-31T05:12:54Z

 import {poll} from '../../../../testing/iframe';

-describes.realWin('amp-selector amp-list interaction', {
+// TODO(kevinkassimo): These tests appear to be failing at HEAD.


@kevinkassimo These tests are failing locally for me on master. Am I missing something?

willchou$ gulp test --nobuild --watch --files=extensions/amp-selector/0.1/test/test-selector-list.js [01:00:01] Using gulpfile ~/Documents/amphtml/gulpfile.js [01:00:01] Starting 'test'... [01:00:01] Run gulp help to see a list of all test flags. [01:00:01] ⤷ Use --nohelp to silence these messages. [01:00:01] ⤷ Use --headless to run tests in a headless Chrome window. [01:00:01] Running tests against unminified code. [01:00:01] --nobuild: Skipping build. [01:00:01] --watch: Enabling watch mode. Editing and saving a file will cause the tests for that file to be re-run in the same browser instance. [01:00:01] --files: Running tests in the file(s): extensions/amp-selector/0.1/test/test-selector-list.js [01:00:01] Setting the runtime's AMP config to prod [01:00:01] Removed existing config from dist/amp.js [01:00:01] Enabled local development mode in dist/amp.js [01:00:01] Wrote prod AMP config to dist/amp.js [01:00:01] Removed existing config from dist/v0.js [01:00:01] Enabled local development mode in dist/v0.js [01:00:01] Wrote prod AMP config to dist/v0.js [01:00:01] Webserver started at http://localhost:31862 [01:00:01] Started test responses server on localhost:31862 [01:00:01] Running test. Press Ctrl + C to cancel... [01:00:14] Running tests locally... START: amp-selector amp-list interaction ● should load selector options correctly with template ● "before each" hook ● "after each" hook for "should not call init again if no actual option is changed" ● "before each" hook ● "after each" hook Finished in 1.437 secs / 0.149 secs @ 01:00:16 GMT-0400 (EDT) SUMMARY: ● 1 test completed ● 4 tests failed FAILED TESTS: amp-selector amp-list interaction ● "before each" hook Chrome 68.0.3440 (Mac OS X 10.13.5) ReferenceError: sandbox is not defined at Context.<anonymous> (Users/willchou/Documents/amphtml/extensions/amp-selector/0.1/test/test-selector-list.js:58:35) ● "after each" hook Chrome 68.0.3440 (Mac OS X 10.13.5) TypeError: Cannot read property 'call' of undefined ● "before each" hook Chrome 68.0.3440 (Mac OS X 10.13.5) Error: Uncaught Error: Missing resource prop on [object HTMLDivElement] The test "amp-selector amp-list interaction should load selector options correctly with template" resulted in a call to console.error. (See above line.) ⤷ If the error is not expected, fix the code that generated the error. ⤷ If the error is expected (and synchronous), use the following pattern to wrap the test code that generated the error: 'allowConsoleError(() => { <code that generated the error> });' ⤷ If the error is expected (and asynchronous), use the following pattern at the top of the test: 'expectAsyncConsoleError(<string or regex>[, number of times the error message repeats]);' (/Users/willchou/Documents/amphtml/src/error.js:211:6 <- /var/folders/5m/xk80sl9x2sd131c_vz9dpmpr006wch/T/ddb86c6c1c0f0fad2e99960e10a22fea.browserify:39872) ● "after each" hook for "should not call init again if no actual option is changed" Chrome 68.0.3440 (Mac OS X 10.13.5) TypeError: Cannot read property 'customElements' of undefined at AmpFixture.teardown (Users/willchou/Documents/amphtml/testing/describes.js:799:13) at http://localhost:9876/Users/willchou/Documents/amphtml/testing/describes.js:347:19 at Array.forEach (<anonymous>) at Context.<anonymous> (Users/willchou/Documents/amphtml/testing/describes.js:346:37)

Seems to be complaining undefined sandbox. Have you tried adding sandbox = env.sandbox; or sandbox = sinon.sandbox.create(); at the beginning of beforeEach?

Might fix the first one... I'll let you figure out the others since you wrote it. 😉

Okay. Seems to be cause by missing Resource registration. Explicitly added through Service and seems now the error messages are gone. Sending a PR to your branch

@choumx Opened a PR on your fork. Sorry for the trouble. Ping me if it still does not work

codecov-io · 2018-07-31T05:41:47Z

Codecov Report

Merging #17209 into master will decrease coverage by 0.02%.
The diff coverage is 54.54%.

@@            Coverage Diff             @@
##           master   #17209      +/-   ##
==========================================
- Coverage   77.85%   77.82%   -0.03%     
==========================================
  Files         562      562              
  Lines       41169    41164       -5     
==========================================
- Hits        32052    32037      -15     
- Misses       9117     9127      +10

Flag	Coverage Δ
#integration_tests	`36.17% <ø> (ø)`	⬆️
#unit_tests	`76.88% <54.54%> (-0.03%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17de093...06e8f6d. Read the comment docs.

Fix test-selector-list.js tests

googlebot · 2018-07-31T14:41:14Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

googlebot · 2018-07-31T14:44:47Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

…7209) * Hide placeholder/fallback/loading after rerender with local data. * Add comments. * Skip failing test-selector-list tests. * Fix test-selector-list.js tests

William Chou added 2 commits July 31, 2018 00:03

Hide placeholder/fallback/loading after rerender with local data.

c7b67e8

Add comments.

86de0d7

dreamofabear requested a review from aghassemi July 31, 2018 04:12

googlebot added the cla: yes label Jul 31, 2018

dreamofabear commented Jul 31, 2018

View reviewed changes

Skip failing test-selector-list tests.

640e616

dreamofabear commented Jul 31, 2018

View reviewed changes

kevinkassimo and others added 2 commits July 30, 2018 23:17

Fix test-selector-list.js tests

86391f0

Merge pull request #1 from kevinkassimo/fix-test-selector-list

06e8f6d

Fix test-selector-list.js tests

googlebot added cla: no and removed cla: yes labels Jul 31, 2018

dreamofabear added cla: yes and removed cla: no labels Jul 31, 2018

rsimha mentioned this pull request Jul 31, 2018

🏗🐛 Print uncaught errors seen during tests (instead of rethrowing them) #17213

Merged

aghassemi approved these changes Jul 31, 2018

View reviewed changes

aghassemi mentioned this pull request Jul 31, 2018

🌸 Cherry pick request for 17209 into 17097 #17215

Closed

dreamofabear merged commit 89f38ca into ampproject:master Jul 31, 2018

dreamofabear deleted the amp-list-reset-bug branch July 31, 2018 16:13

Navigation Menu

Uh oh!

amp-list: Hide placeholder et al. after re-render with local data - #17209

amp-list: Hide placeholder et al. after re-render with local data#17209
dreamofabear merged 5 commits into
ampproject:masterfrom
dreamofabear:amp-list-reset-bug

dreamofabear commented Jul 31, 2018 •

edited

Loading

Uh oh!

dreamofabear Jul 31, 2018

Uh oh!

rsimha Jul 31, 2018 •

edited

Loading

Uh oh!

dreamofabear Jul 31, 2018

Uh oh!

dreamofabear Jul 31, 2018

Uh oh!

kevinkassimo Jul 31, 2018

Uh oh!

dreamofabear Jul 31, 2018

Uh oh!

kevinkassimo Jul 31, 2018

Uh oh!

kevinkassimo Jul 31, 2018

Uh oh!

codecov-io commented Jul 31, 2018 •

edited

Loading

Uh oh!

googlebot commented Jul 31, 2018

Uh oh!

googlebot commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

dreamofabear commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dreamofabear Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

rsimha Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

dreamofabear Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

kevinkassimo Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

dreamofabear Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

kevinkassimo Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

kevinkassimo Jul 31, 2018

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

googlebot commented Jul 31, 2018

Uh oh!

googlebot commented Jul 31, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dreamofabear commented Jul 31, 2018 •

edited

Loading

rsimha Jul 31, 2018 •

edited

Loading

codecov-io commented Jul 31, 2018 •

edited

Loading