Skip to content
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

Introduce video-ad-start/end event #23733

Merged
merged 1 commit into from Aug 6, 2019

Conversation

@zhouyx
Copy link
Contributor

commented Aug 6, 2019

Closes #13753

@zhouyx zhouyx requested review from alanorozco and cvializ Aug 6, 2019

@googlebot googlebot added the cla: yes label Aug 6, 2019

@zhouyx zhouyx added this to To do in Analytics Fixit (2019) via automation Aug 6, 2019

@zhouyx zhouyx moved this from To do to In progress in Analytics Fixit (2019) Aug 6, 2019

@cvializ
cvializ approved these changes Aug 6, 2019
Copy link
Contributor

left a comment

LGTM!

@cvializ

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

Should there be tests for this in extensions/amp-ima-video/0.1/test/test-amp-ima-video.js?

@zhouyx

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Thank you for the quick review! This is not a feature that's specific to ima video.
I thought about adding test coverage to test-video-manager.js but doesn't seem to find test coverage for other analytics event. Please let me know what's a good place to add generic video analytics test coverage. Thanks.

@cvializ

This comment has been minimized.

Copy link
Contributor

commented Aug 6, 2019

general video tests should go in video manager, and if possible it should be added to the individual video elements that support it

@zhouyx

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@rsimha I noticed one more issue with gulp watch while testing the video components.

video-manager-impl.js lives under src/service folder. Which means we are not monitoring the file to rebuild the component. However the file is also not built to v0.js.
When I was testing the change locally, a change to video-manager-impl.js won't be picked up, and I had to restart the gulp build myself multiple times.

@cvializ wonder how did you test the change to the above file previous. and @rsimha for ideas to fix this. Thanks.

@rsimha

This comment has been minimized.

Copy link
Collaborator

commented Aug 6, 2019

@rsimha I noticed one more issue with gulp watch while testing the video components.

video-manager-impl.js lives under src/service folder. Which means we are not monitoring the file to rebuild the component. However the file is also not built to v0.js.
When I was testing the change locally, a change to video-manager-impl.js won't be picked up, and I had to restart the gulp build myself multiple times.

Currently, we pick up entry-point-level changes for core runtime code, and directory level changes for extensions. This file appears to be violating both conventions.

if (options.watch) {
bundler = watchify(bundler);
bundler.on('update', () => performBundle(/* continueOnError */ true));
}

// Use a separate watcher for extensions to copy / inline CSS and compile JS
// instead of relying on the watcher used by compileUnminifiedJs, which only
// recompiles JS.
if (options.watch) {
options.watch = false;
watch(path + '/*', function() {
buildExtension(
name,
version,
latestVersion,
hasCss,
Object.assign({}, options, {continueOnError: true})
);
});
}

Couple options:

  • Is it reasonable to move video-manager-impl.js into its extension directory?
  • If not, is there a good way to associate an extension with all the files that it uses?
@zhouyx

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

Talked with @cvializ offline. Decided we will not be adding test for 3p code within iframe.

@zhouyx zhouyx merged commit 0b06cb1 into ampproject:master Aug 6, 2019

14 of 16 checks passed

ampproject/owners-check ampproject/owners-check
Details
ampproject/pr-deploy Ready to create a test site.
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
ampproject/bundle-size Δ -0.01KB | no approval necessary
Details
ampproject/tests/e2e (local) 308 tests passed
Details
ampproject/tests/integration (local) 303 tests passed
Details
ampproject/tests/integration (saucelabs) 449 tests passed
Details
ampproject/tests/integration (single-pass) 245 tests passed
Details
ampproject/tests/unit (local) 10034 tests passed
Details
ampproject/tests/unit (local-changes) 283 tests passed
Details
ampproject/tests/unit (saucelabs) 7837 tests passed
Details
cla/google All necessary CLAs are signed
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +20.76% compared to b24583a
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
percy/amphtml Visual review automatically approved, no visual changes found.
Details

Analytics Fixit (2019) automation moved this from In progress to Done Aug 6, 2019

@zhouyx

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2019

@rsimha I believe we want to keep the file there because it's shared across all the video components. That those video components will install the video manager. @cvializ to keep me honest.

If not, is there a good way to associate an extension with all the files that it uses?

I'm thinking about the same. One easy workaround is to hard code the files when the component is identified as a video component.

@zhouyx zhouyx deleted the zhouyx:video-ad-events branch Aug 6, 2019

@zhouyx zhouyx moved this from Done to PRs in Analytics Fixit (2019) Aug 8, 2019

@zhouyx zhouyx moved this from PRs to Done in Analytics Fixit (2019) Aug 9, 2019

thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants
You can’t perform that action at this time.