-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Automated functional tests for longform testpages #3659
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. All tests passed in first attempt. Few minor changes suggested.
We should change command to gulp test --e2e
.
To test in IE 11, maybe provide some flag to only run tests in IE 11. gulp test --e2e --ie11
? Main command will run without IE.
|
||
</script> | ||
<script type="text/javascript"> | ||
function getIndustry(id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should create common.js and move all functions in that file ? They are same for all the long form pages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will create this and update the pages.
<script type="text/javascript" src="https://mssl.fwmrm.net/libs/adm/6.24.0/AdManager-debug.js"></script> | ||
<!-- <script type="text/javascript" src="player.js"></script> --> | ||
<style type="text/css"> | ||
#videoPlayer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move css to separate file style.css
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do.
this.retries(3); | ||
it('process the bids successfully', function() { | ||
browser | ||
.url('http://test.localhost:9999/integrationExamples/longform/basic_w_custom_adserver_translation.html?pbjs_debug=true') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if test.localhost is not defined in my hosts file ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will test this and determine if it's an issue.
@jaiminpanchal27 I pushed in a set of updates based on your feedback; please take another look when you have the chance. |
@@ -1,65 +1,65 @@ | |||
{ | |||
"bs_ie_14_windows_10": { | |||
"bs_edge_16_windows_10": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes are going to impact unit tests as well correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new browsers would, but the unit tests are still running successfully.
@mkendall07 Based on what we discussed today, I have added IE11 back into the Both sets of tests are working. Please review when you have the chance. |
LGTM. Although looks like circlecI failed. I restarted the build |
gulpfile.js
Outdated
@@ -244,6 +231,13 @@ function newKarmaCallback(done) { | |||
function test(done) { | |||
if (argv.notest) { | |||
done(); | |||
} else if (argv.lfe2e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you change this to e2e?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed a new commit with the updates.
* initial changes for longform e2e testing * added old safari browsers back, test spec file, and updated test pages * add retries settings to e2e tests, lower timeout thresholds * remove commented code * additional clean-up * update browser versions for more stable e2e tests * support host param in gulp command and other updates * refactor how host param was handled * add ie11 to browsers but remove it for e2e tests * update gulp task name to e2e-test * update e2e variable
* initial changes for longform e2e testing * added old safari browsers back, test spec file, and updated test pages * add retries settings to e2e tests, lower timeout thresholds * remove commented code * additional clean-up * update browser versions for more stable e2e tests * support host param in gulp command and other updates * refactor how host param was handled * add ie11 to browsers but remove it for e2e tests * update gulp task name to e2e-test * update e2e variable
Type of change
Description of change
This PR adds a set of functional tests to validate some longform test pages.
The tests just focus on the Prebid objects that are generated (ie the bids and the targeting keys), not the ads playing through the video player itself. The auction request is controlled through a button object on the pages so that it would allow enough time for the page to fully initialize (especially the longform category data stored in localStorage).
Each of the tests follow the basic steps:
Load Prebid
button to appear and then click it (to run the auction)The command to run these tests is:
gulp lfe2e
(name can be changed)NOTE The tests are configured to use a number of retries in case their first attempt fails. Some of the browserstack browsers (namely IE11 now) have been a bit flaky while trying to load these pages. For example, they'll load the 3 out of the 4 pages in the same browser without issue, but then fail to load it for the last page (the error shown in the browser implies there was a connection issue or not found). Then when you rerun the suite, everything would work fine.
ADDITIONAL NOTE As part of this PR, I removed the old E2E test code from the project to avoid confusion between two sets of e2e tests (since it wasn't completely working anyways).