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.

Sign up for GitHub

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

Jump to bottom

Randomize pbjs global name when testing #1787

Merged
merged 2 commits into from
Nov 28, 2017
Merged

Randomize pbjs global name when testing #1787

merged 2 commits into from
Nov 28, 2017

Conversation

mattpr
Copy link
Contributor

@mattpr mattpr commented Oct 31, 2017

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other

Description of change

It seems that adapter tests routinely hard-code pbjs instead of using $$PREBID_GLOBAL$$. I suppose things will change with prebid-1.0, but meanwhile this change forces those tests to break by randomizing the global name when testing.

There is probably a cleaner way to do this, but I also didn't want to do a bigger refactor given the plans to move away from using $$PREBID_GLOBAL$$.

References

#1786
#1666

@mattpr mattpr changed the title Randomize pbjs global on test Randomize pbjs global name when testing Oct 31, 2017
@mkendall07
Copy link
Member

The tests failed on this PR. I restarted the job to see if it will pass or not. If it doesn't pass again please rebase off master.

@mattpr
Copy link
Contributor Author

mattpr commented Nov 16, 2017

The tests passed when I opened the PR and are passing now. No idea why the runs in the middle failed.

mkendall07
mkendall07 approved these changes Nov 27, 2017
View reviewed changes
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.

LGTM

jsnellbaker
jsnellbaker approved these changes Nov 28, 2017
View reviewed changes
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

@jsnellbaker jsnellbaker merged commit f9cb0dd into prebid:master Nov 28, 2017
jsnellbaker added a commit that referenced this pull request Nov 28, 2017
@jsnellbaker
Revert "Randomize pbjs global name when testing (#1787)"
c90e459 
This reverts commit f9cb0dd.
@jsnellbaker jsnellbaker mentioned this pull request Nov 28, 2017
Merged
jsnellbaker added a commit that referenced this pull request Nov 28, 2017
@jsnellbaker
Revert "Randomize pbjs global name when testing" (#1891)
d302a87 
* Revert "Postbid bugfix  (#1844)"

This reverts commit 2e7783a.

* Revert "Populate adtypes array (#1862)"

This reverts commit 214ba19.

* Revert "Pass bidderRequestId parameter in Adform adapter (#1888)"

This reverts commit 59800f4.

* Revert "Add Arteebee bidder adapter (#1849)"

This reverts commit 18fe42d.

* Revert "fix linting errors for trailing spaces (#1889)"

This reverts commit d41287a.

* Revert "Randomize pbjs global name when testing (#1787)"

This reverts commit f9cb0dd.
@jsnellbaker
Copy link
Collaborator

To note - this PR was reverted in lieu of some issues we started to encounter for the build process after this was originally merged.

@jsnellbaker jsnellbaker removed the LGTM label Nov 29, 2017
@mattpr
Copy link
Contributor Author

mattpr commented Dec 10, 2017

Can you guys share any more info about the build issues that came up post-merge? Happy to look at improving this PR.

@mkendall07
Copy link
Member

It put the build always in test mode. So production build wouldn't be pbjs variable.

@mattpr
Copy link
Contributor Author

mattpr commented Dec 11, 2017

Interesting. gulp build doesn't run the karma tests for me and so the test flag isn't set and the resulting build uses the global pbjs name instead of the random name. Can you give me an example of your production build command since I am guessing it isn't gulp build?

@mkendall07
Copy link
Member

I see. No the issue was when running gulp serve - this puts us into a development mode to test and build code. I believe it's also what we use to run our browserstack tests before release which is what failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkendall07 mkendall07 mkendall07 approved these changes

@jsnellbaker jsnellbaker jsnellbaker approved these changes

@protonate protonate Awaiting requested review from protonate

Assignees

@mkendall07 mkendall07

@jsnellbaker jsnellbaker

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mattpr @mkendall07 @jsnellbaker @protonate