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

Match port when testing legacy browser #1511

Merged
merged 1 commit into from
Aug 22, 2017
Merged

Match port when testing legacy browser #1511

merged 1 commit into from
Aug 22, 2017

Conversation

matthewlane
Copy link
Collaborator

@matthewlane matthewlane commented Aug 21, 2017

Type of change

  • Testfix

Description of change

The parse function in urls.js returns a port of 443 when tested in IE11, blank in other browsers. A hardcoded url that is matched against doesn't contain this port and causes the test mentioned in #1302 to fail. This PR adds that port to the hardcoded string when tested in IE11 and fixes #1302.

@dbemiller
Copy link
Contributor

Are you sure this is safe? The test is titled should return original adservertag if there are no bids for the given placement code.

I'm not sure why the original author thought this was an important test... but assuming that they were right, wouldn't this be a code bug?

@mkendall07
Copy link
Member

well this just adds the port number if the browser is IE 11, assuming this is right behavior for IE, since this implementation inside of url is based on the anchor tag, which is implementation specific.

@dbemiller
Copy link
Contributor

I'll merge it cause it's testing a deprecated method, and it fixes our CI :/

...but in general, we should be normalizing browser differences at as low a level as possible. jQuery was pretty much built on the idea... and consistency is the main motivation for the push for web standards at all.

If the implementation is browser-dependent, then I'd consider it a buggy implementation.

@dbemiller dbemiller merged commit 6572cc7 into master Aug 22, 2017
@dbemiller dbemiller deleted the ie11-port-testfix branch August 22, 2017 16:34
outoftime pushed a commit to Genius/Prebid.js that referenced this pull request Aug 24, 2017
…built

* 'master' of https://github.com/prebid/Prebid.js: (83 commits)
  feat(strAdapt): check if tagJS is already present (prebid#1500)
  Updated karma-mocha, and simplified the test framework for runs which dont include --watch. (prebid#1520)
  Revert "drop specific code for index adapter (prebid#1487)" (prebid#1529)
  Override default asset params when set on ad unit (prebid#1524)
  Adding new kv to xhb Adapter (prebid#1513)
  removing for...of loops because IE cannot handle them properly (prebid#1523)
  Increment pre version
  Prebid 0.27.0 Release
  Move unit test file to appropriate location (prebid#1516)
  Support 'cta' native asset (prebid#1505)
  Add adapter parameter types (prebid#1504)
  Register bid adapter (prebid#1514)
  Match port when testing legacy browser (prebid#1511)
  modify 'featureforward' adapter to be an indepandant adapter (prebid#1288)
  Feature/s2s client side fallback (prebid#1485)
  Make bid.vastUrl use the cache URL if the bid didnt already have one. (prebid#1506)
  OpenX Adapter Update: (prebid#1438)
  Add session id feature for roxot analytics adapter (prebid#1498)
  Update platformioBidAdapter (prebid#1493)
  Adding VAST Payload support for video bids (prebid#1407)
  ...
ptomasroos pushed a commit to happypancake/Prebid.js that referenced this pull request Aug 25, 2017
philipwatson pushed a commit to mbrtargeting/Prebid.js that referenced this pull request Sep 18, 2017
jbAdyoulike pushed a commit to jbAdyoulike/Prebid.js that referenced this pull request Sep 21, 2017
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fragile test fails in prebid-core on IE11
3 participants