Improving kargo unit tests for currency handling #3106
Conversation
samuelhorwitz
changed the title
|
I looked over your test changes and they seem good. In terms of your questions, there's nothing at present in the project that does a full e2e type of testing in an automated fashion. This may be something that's looked at in the future, but I don't know of any definite plans yet. In the meantime - these types of checks normally performed by the reviewers, who would checkout the PR's branch and test it locally to verify the functionality. Ideally the original issue would have been observed much earlier (when the code was implemented) so it wouldn't have been a surprise, but that's not how it turned out. :( As you noted, mocking the currency config is the ideal here as your unit tests should focus on how your adapter code is working. The mocks though do need to represent how that feature is actually working - so that the proper responses /objects are used in the tests and the wrong assumptions aren't made. Once it's set though, it should be reliable. The only times it might be changed is during a major version upgrade, where breaking changes may be incorporated into the project. But these are announced and propagated so as to let everyone know what changes are coming and allow them to adjust. |
* 'master' of https://github.com/prebid/Prebid.js: (367 commits) Rubicon adapter: get referrer from bidderRequest.refererInfo.referer; (prebid#3087) Minor freewheel-ssp update (prebid#3119) fixes prebid#3128 YieldlabBidAdapter is not using bidRequest.params.adSize (prebid#3129) Support Video Renderer (prebid#3104) Fix for Issue 3130: passing new copy of adUnits object to every adapter (prebid#3131) Add video params to Beachfront adapter (prebid#3121) Sonobi - Fix ref encoding (prebid#3125) update circleci link to just Prebid.js builds (prebid#3132) Bugfix: Issue 3111 (prebid#3122) increment prebid version Prebid 1.25.0 Release adding account to s2s bidder-sync request (prebid#3123) Revert "Trafficroots Bid Adapter Submission (prebid#2993)" (prebid#3124) Trafficroots Bid Adapter Submission (prebid#2993) add versioning and deprecation policy doc (prebid#3103) improving kargo unit tests for currency handling (prebid#3106) AdOcean adapter improvment (prebid#3011) Serverbid Bid Adapter: Add pubnx alias (prebid#3064) Adds an id parameter (prebid#3107) added sizes for rubicon (prebid#3094) ...
Type of change
Description of change
Kargo was sort of blindsided by #2923 which went out with 1.19.0 and broke our adapter on that version.
I fixed this when it was pointed out to me here: #2958 (comment)
That fix went out with 1.20.0.
I am now updating the tests to better support this as I was expected an incorrect shape for currency from the config. I also make sure all possible paths are tested (if no currency object, if no ad server currency key should both have sane default of "USD").
This is the kind of thing that boils down to me misunderstanding the shape of a config a year ago, and no issues arising from it until more stringent external checks were added. Unit testing wouldn't really catch it as I believe my practice of mocking the config for the unit tests is correct (although my incorrect mocking due to human error/misunderstanding is obviously wrong).
Anyone have any ideas how stuff like this could be avoided in the future? Is there an automated integration or E2E testing in the pipeline for adapters to contribute to?