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

replace all xhr stubs with global xhr stub to prevent all requests #4687

Merged
merged 12 commits into from
Jan 7, 2020

Conversation

snapwich
Copy link
Collaborator

@snapwich snapwich commented Jan 2, 2020

Type of change

  • Bugfix
  • Refactoring (no functional changes, no api changes)

Description of change

Replaces all sinon fake xhr and fake server stubbing with global stub to prevent any XMLHttpRequests from happening.

Update all pixels to use utils.triggerPixel and properly stub in tests.

Other information

Work related to #4348

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@snapwich Thanks for putting this all together.

I think the setup looks fine, though when I checked out this PR and checked the project, there appear to be a handful of other files that are using their own XHR mocks. Can you resync with master (if needed) and update these other references?

To help out, I put together a list of the files that I found:
adagioAnalyticsAdapter_spec.js
invisiblyAnalyticsAdapter_spec.js
liveIntentIdSystem_spec.js
prebidServerBidAdapter_spec.js (there's a set of tests around line 1632 that still uses it)
ucfunnelAnalyticsAdapter_spec.js
userId_spec.js
yuktamediaAnalyticsAdaptor_spec.js

@jsnellbaker jsnellbaker self-assigned this Jan 3, 2020
@jsnellbaker jsnellbaker added the needs 2nd review Core module updates require two approvals from the core team label Jan 3, 2020
@snapwich snapwich removed the needs 2nd review Core module updates require two approvals from the core team label Jan 3, 2020
@snapwich
Copy link
Collaborator Author

snapwich commented Jan 3, 2020

thanks @jsnellbaker, just pushed a fix for those.

this isn't quite ready for review yet, still in progress. hoping to find some fixes for the other issues listed in #4348 as well (script includes, pixels firing, etc).

@snapwich
Copy link
Collaborator Author

snapwich commented Jan 3, 2020

should be ready for review. fixes all external requests other than pixels (which are low priority because they don't load executable code, but should probably be fixed at some point).

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

Hi @snapwich

Thanks for making the extra updates. When you have the chance could you make two additional changes?

  • update the yuktamediaAnalyticsAdaptor_spec.js test file as part of this clean-up.
  • add a small blurb to the CONTRIBUTING.md around the Test Guidelines about the XHR mock (not sure how often people look at this, but it'll still be good to have it documented).

Thanks!

@jsnellbaker jsnellbaker added needs 2nd review Core module updates require two approvals from the core team needs update and removed needs review labels Jan 6, 2020
@snapwich
Copy link
Collaborator Author

snapwich commented Jan 6, 2020

@jsnellbaker updated

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the work here.

Copy link
Member

@mkendall07 mkendall07 left a comment

Choose a reason for hiding this comment

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

Awesome work Rich! thanks for helping out with this.

@jsnellbaker jsnellbaker merged commit cb3a856 into master Jan 7, 2020
redaguermas added a commit to redaguermas/Prebid.js that referenced this pull request Jan 8, 2020
…idVersion1.2.0

* 'master' of https://github.com/prebid/Prebid.js: (22 commits)
  fix lint errors in unit test file (prebid#4702)
  Add Revcontent Adapter (prebid#4654)
  Changed data structure in Platform One Analytic Adapter (prebid#4647)
  increment pre version
  Prebid 3.2.0 Release
  Add static API option to the consentManagementUsp module. (prebid#4685)
  replace all xhr stubs with global xhr stub to prevent all requests (prebid#4687)
  Add CCPA us_privacy support to spotxBidAdapter (prebid#4689)
  ucfunnel adapter support CCPA and remove utils.js in adapter (prebid#4541)
  freewheelSSPBidAdapter  (prebid#4645)
  Add CCPA support to Beachfront adapter (prebid#4673)
  add seedingAlliance Adapter (prebid#4614)
  Changed analytics data structure in YuktaMedia Analytic Adapter (prebid#4659)
  Add eplanning adapter for prebid 3.0 compliant and CCPA and GDPR support (prebid#4643)
  Bidder schain support (prebid#4551)
  Added CCPA support and GDPR compliance to Cedato adapter (prebid#4683)
  pass us privacy consent string to request (prebid#4581)
  Prebid 3 Admixer (prebid#4615)
  Pass uspConsent in bidRequest (prebid#4675)
  Advertly: New Bidder Adapter Submission (prebid#4496)
  ...
tadam75 pushed a commit to smartadserver/Prebid.js that referenced this pull request Jan 9, 2020
…rebid#4687)

* replace all xhr stubs with global xhr stub to prevent all requests

* fix test failures after conflict resolution

* removed rest of custom XMLHttpRequest stubs

* update adloader stub to be present even inbetween tests

* remove unnecessary console.log in tests

* third-party scripts loaded with loadExternalScript

* update new test files to use mocked xhr server

* add blurb about using global xhr stub for contributors

* stub videoNow external requests

* update all pixels to use utils.triggerPixel and stub tests

* remove console.log out of freewheel test

* simplify userId test setup and teardown
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs 2nd review Core module updates require two approvals from the core team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants