1298 fix fbf0840 #2067
Merged
1298 fix fbf0840 #2067
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…n addBidResponse * bidmanager: move addBidResponse helper functions out of closures
In order to ensure the calling order is correct, call doCallbacks from within addBidToAuction. XXX: This breaks the test that's removed in this patch. The reason it breaks is that the TIMEOUT event is not emitted if all bids have been returned before the timeout handler is called. Previously the bid manager was rejecting the bid if the bid triggered the timeout handler, which doesn't really make much sense. The test SHOULD work if there are two bidders, as the first response should trigger the timeout. I had a hard time adding a second bidder to the test code, so I was hoping to get some help.
|
@kitwestneat Thanks for adding this back. I missed that earlier. Regarding tests we are working on it. With this #2014 we are updating the stack and increasing the speed to run the tests on local and browserstack. |
jaiminpanchal27
approved these changes
Feb 2, 2018
matthewlane
approved these changes
Feb 6, 2018
Millerrok
pushed a commit
to Vertamedia/Prebid.js
that referenced
this pull request
Feb 12, 2018
* 'master' of https://github.com/prebid/Prebid.js: (51 commits) Update maintainer email (prebid#2132) Replace event string with constant (prebid#2128) Add adapter for IAS (prebid#2056) Fix sovrn dealid (prebid#2119) Added support for NURL and ADM as backup (prebid#2112) fix bug where hooked functions w/ no hooks weren't ran immediately (prebid#2115) Increment pre version Prebid 1.3.1 Release Omit app and device if not present rather than send false (prebid#2116) Increment pre version Prebid.js 1.3.0 Release Unit test fixes (prebid#2111) must explicitly list pre1api for it to be included in build (prebid#2097) adkernel adapter additional bid parameters (prebid#2105) removing userSync (prebid#2032) 33across Bid Adapter: Bugfix + Refactor (prebid#2024) Port calling order fix to 1.x (prebid#2067) Update adform adapter request (prebid#2107) Serverbid Bid Adapter: getUserSyncs and new adsizes (prebid#2106) Add hfa and pv parameter to request payload (prebid#2109) ...
kitwestneat
added a commit
to kitwestneat/Prebid.js
that referenced
this pull request
Feb 15, 2018
snapwich
pushed a commit
that referenced
this pull request
Mar 2, 2018
* refactor auctionmanager_spec.js and getKeyValueTargetingPairs consolidate duplicated code into helper functions. * re-add timeout test removed in PR #2067 to auctionmanager_spec.js
dluxemburg
pushed a commit
to Genius/Prebid.js
that referenced
this pull request
Jul 17, 2018
* * bidmanager: pass closure variables explicitly to helper functions in addBidResponse * bidmanager: move addBidResponse helper functions out of closures * WIP: call doCallbacks from addBidToAuction In order to ensure the calling order is correct, call doCallbacks from within addBidToAuction. XXX: This breaks the test that's removed in this patch. The reason it breaks is that the TIMEOUT event is not emitted if all bids have been returned before the timeout handler is called. Previously the bid manager was rejecting the bid if the bid triggered the timeout handler, which doesn't really make much sense. The test SHOULD work if there are two bidders, as the first response should trigger the timeout. I had a hard time adding a second bidder to the test code, so I was hoping to get some help.
dluxemburg
pushed a commit
to Genius/Prebid.js
that referenced
this pull request
Jul 17, 2018
* refactor auctionmanager_spec.js and getKeyValueTargetingPairs consolidate duplicated code into helper functions. * re-add timeout test removed in PR prebid#2067 to auctionmanager_spec.js
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a work in progress, I was hoping to get some feedback on the tests.
Type of change
Description of change
This is a port of fbf0840 to 1.0.
The calling order for addBidToAuction and doCallbacksIfNeeded is not consistent. This patch moves most of the calls to doCallbacks into addBidToAuction to make sure it is used consistently.
There is a problem with one of the tests though. A side effect of this change is that the bid that triggers the timeout is added to the bids received list. This seems appropriate to me since if you've waited for and received the bid, it doesn't make sense to throw it away because of timing. However, if that bid is the last bid that prebid was waiting for, the timeout event is not triggered. The test case that I removed in this patch expects that any bid received after the timeout window will trigger the timeout event.
It seems like a fix could be to just add a second bidder so that the triggering bid is not the last bid expected. I tried modifying the test to do that, but ran into a lot of issues getting the test framework to bend to my will. I can bang on it some more, but I thought I'd see if anyone who knows the test system better could help.
As more of a general question, have you all thought about working on the test framework to make it easier to use? There is a lot of duplicated code and partially mocked objects, which makes it confusing to work with. It'd be nice if it were easier to get a standard mocked request / response, and the test could modify it as needed.
The current situation is frustrating and there are times that I don't even submit bugfix patches because the tests are so brittle and difficult to change, like I spend 10x time on wrangling tests vs the actual bugfix. Anyway just my 2c, /rant. Would you all be open to patches to refactor the tests?