Skip to content

feat(ads): allow adsResponse for making ad request#2946

Merged
TheModMaker merged 2 commits intoshaka-project:masterfrom
DDeis:ima-adsresponse
Oct 29, 2020
Merged

feat(ads): allow adsResponse for making ad request#2946
TheModMaker merged 2 commits intoshaka-project:masterfrom
DDeis:ima-adsresponse

Conversation

@DDeis
Copy link
Contributor

@DDeis DDeis commented Oct 28, 2020

Currently, shaka throw an error if adTagUrl is not set in imaRequest.
This add the possibility to use a VAST document directly using adsResponse property instead of making a request with an ad tag url.

Resolves #2892

Currently, shaka throw an error if adTagUrl is not set in imaRequest.
This add the possibility to use a VAST document directly using adsResponse property instead of making a request with an ad tag url.

Resolves shaka-project#2892
@DDeis
Copy link
Contributor Author

DDeis commented Oct 28, 2020

I'm not sure if that's necessary, I wanted to add unit in ad_manager_unit.js to test shaka.ads.ClientSideAdManager.requestAds but I got stuck in the process.

I added properties to IMA mock, but I don't know how to mock shaka.ads.ClientSideAdManager.eventManager_.
This may be the wrong way of doing it, maybe should I create a separate unit test file for shaka.ads.ClientSideAdManager ?

Copy link
Contributor

@TheModMaker TheModMaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs a test. It might be nice to have an integration test to verify IMA allows the adTagUrl to be not set, but we don't have any other integration tests.

@DDeis
Copy link
Contributor Author

DDeis commented Oct 28, 2020

I don't think this needs a test. It might be nice to have an integration test to verify IMA allows the adTagUrl to be not set, but we don't have any other integration tests.

I can try to make an integration for that but this is not my forte, I will need some help for that.
For instance, how to load IMA script ?

@shaka-bot
Copy link
Collaborator

Test Failure:

Generating Closure dependencies...
Linting JavaScript...
Linting CSS...
Linting HTML...

Config loaded: /var/lib/jenkins/workspace/Manual PR Test (local-tests)/.htmlhintrc

Config loaded: /var/lib/jenkins/workspace/Manual PR Test (local-tests)/.htmlhintrc

Config loaded: /var/lib/jenkins/workspace/Manual PR Test (local-tests)/.htmlhintrc

Scanned 3 files, no errors found (62 ms).
Checking that the build files are complete...
Checking for common misspellings...
Checking correct usage of eslint-disable...
Checking the tests for type errors...
No changes detected, skipping. Use --force to override.
/var/lib/jenkins/workspace/Manual PR Test (local-tests)/externs/ima.js:35:14: ERROR - [JSC_UNRECOGNIZED_TYPE_ERROR] Bad type annotation. Unknown type google.ima.AdsRequest
35|   /** @param {google.ima.AdsRequest} request */
^

/var/lib/jenkins/workspace/Manual PR Test (local-tests)/externs/ima.js:230:24: ERROR - [JSC_TYPE_PARSE_ERROR] Bad type annotation. expected closing } See https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler for more information.
230|  *   adsResponse: string|undefined,
^

/var/lib/jenkins/workspace/Manual PR Test (local-tests)/externs/shaka/ads.js:58:14: ERROR - [JSC_UNRECOGNIZED_TYPE_ERROR] Bad type annotation. Unknown type google.ima.AdsRequest
58|    * @param {!google.ima.AdsRequest} imaRequest
^

/var/lib/jenkins/workspace/Manual PR Test (local-tests)/lib/ads/client_side_ad_manager.js:84:14: ERROR - [JSC_UNRECOGNIZED_TYPE_ERROR] Bad type annotation. Unknown type google.ima.AdsRequest
84|    * @param {!google.ima.AdsRequest} imaRequest
^

/var/lib/jenkins/workspace/Manual PR Test (local-tests)/lib/ads/client_side_ad_manager.js:88:42: ERROR - [JSC_STRICT_INEXISTENT_PROPERTY] Property adsResponse never defined on imaRequest
88|         imaRequest.adTagUrl || imaRequest.adsResponse,
^^^^^^^^^^^

5 error(s), 0 warning(s), 94.5% typed
Build failed
END-BUILD: FAILURE
Build step 'Execute shell' marked build as failure

@shaka-bot
Copy link
Collaborator

All tests passed!

@TheModMaker TheModMaker merged commit 40d4451 into shaka-project:master Oct 29, 2020
@DDeis DDeis deleted the ima-adsresponse branch November 4, 2020 22:03
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 25, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

status: archived Archived and locked; will not be updated

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Ads] allow requesting ads with adsResponse instead of adTagUrl

3 participants