fixed #1246 and add unit test #1427
Conversation
mkendall07
changed the title
| * @param {string} key | ||
| * @returns {${key_value}: ${groupByArray}, key_value: {groupByArray}} | ||
| */ | ||
| export function groupBy(xs, key) { |
There was a problem hiding this comment.
hooray! Nice general code.
This is a bit subjective... but if you want to stretch it even more, you could make this function much more powerful (and only a little bit more complicated) if you accept an (element) => key function here, rather than a raw "key".
Your call would replace groupBy($$PREBID_GLOBAL$$._bidsReceived, 'adUnitCode')
with groupBy($$PREBID_GLOBAL$$._bidsReceived, bid => bid.adUnitCode).
In exchange, this function would work with arrays of anything.
There was a problem hiding this comment.
sure, but I don't understand how this is useful to me?
There was a problem hiding this comment.
lol, it's not. Sorry... should have been clear: I didn't actually expect a change here. More of a fun FYI.
Many general-purpose utils libraries have this function, including lodash, async, the scala stdlib, etc. I was just pointing out how close this comes to "the bleeding edge" of general-purpose code.
test/spec/core/targeting_spec.js
Outdated
| @@ -0,0 +1,81 @@ | |||
| import { expect } from 'chai'; | |||
There was a problem hiding this comment.
Regarding the file location... could we make an effort to group the unit tests into one folder? In this case, tests/spec/core => tests/spec/unit/core
In the current organization, I'd expect to find integration tests (for example, connecting videoCache to prebid-cache) right next to unit tests like this one. This is bad for automated testing, because we'll want different subsets of [unit tests, integration tests, nightwatch/selenium tests] to run at different times during the dev, PR, and Release processes.
src/targeting.js
Outdated
| return { | ||
| [bid.adUnitCode]: getTargetingMap(bid, standardKeys.filter( | ||
| key => typeof bid.adserverTargeting[key] !== 'undefined') // mainly for possibly | ||
| // unset hb_deal |
There was a problem hiding this comment.
Can we clear out the stale comments? I have no idea what these two mean.
There was a problem hiding this comment.
Yeah, although it's not stale. It's saying that hb_deal isn't always defined even though it's in the default key set. I think it should be obvious from the code though.
|
@dbemiller ready for re-review Edit: nevermind, looks like moving the spec file caused the unit test to fail :( |
|
That... sucks :/. Something similar happened to me when I added In that case, it was because that test didn't set up all the global state required by the code it was testing. I fixed it by setting it in the In |
There was a problem hiding this comment.
I looked into the issue with $$PREBID_GLOBAL$$.setConfig vs. config.setConfig.
The core issue is that each _spec file gets its own webpack bundle, but $$PREBID_GLOBAL$$ lives on the window, which is shared between tests.
I ran some tests, and it looks like we can fix this by building a single bundle to run the tests. It also cuts our test time down to ~15 seconds... which is awesome.
...unfortunately, it also results in 100+ test failures. So while it's a great idea, we've got a long way to go before we get there.
All that to say... this looks good for now.
There was a problem hiding this comment.
Was able to reproduce original error in #1246, and then get expected result of highest bid consistently selected in sendAllBids mode with this fix. LGTM
* fixed prebid#1267 and add unit test * remove markup * move unit tests * remove comment * interim fix * fix unit tests. Not sure about having to import config directly in the spec file
* fixed prebid#1267 and add unit test * remove markup * move unit tests * remove comment * interim fix * fix unit tests. Not sure about having to import config directly in the spec file
* fixed prebid#1267 and add unit test * remove markup * move unit tests * remove comment * interim fix * fix unit tests. Not sure about having to import config directly in the spec file
Description of change
Fixes #1246
edit: This fixes #1246 not 1267. 1267 is related but not resolved in this PR.
Other information
@prebid/core
Cc: @harpere @bretg