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

[OIL-190] IE10 e2e test upgrades #150

Merged
merged 30 commits into from
Jun 22, 2018
Merged

[OIL-190] IE10 e2e test upgrades #150

merged 30 commits into from
Jun 22, 2018

Conversation

phogel
Copy link
Contributor

@phogel phogel commented Jun 21, 2018

Fixing tests for IE10. CPC and POI tests fail because they rely on the backend which does not have the required polyfills yet.

@phogel phogel changed the title [Oil 190 browserstacks [OIL-190] IE10 e2e test upgrades Jun 21, 2018
@coveralls
Copy link

coveralls commented Jun 21, 2018

Pull Request Test Coverage Report for Build 292

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 875 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.8%) to 25.186%

Files with Coverage Reduction New Missed Lines %
dist/oil.bundle.js 875 25.19%
Totals Coverage Status
Change from base Build 264: 0.8%
Covered Lines: 898
Relevant Lines: 2930

💛 - Coveralls

@phogel phogel requested a review from ltparis2018 June 21, 2018 11:25
@@ -170,6 +170,10 @@ export function getLanguage() {
}

export function getLanguageFromLocale(localeVariantName) {
if(!localeVariantName) {
logError('localeId missing in locale');
localeVariantName = 'en';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ltparis2018 can you confirm if it is OK to pass a default here? It does not break any test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is ok. OIL layer's default language is english. But the error message needs an improvement I think. What is locale id in this context? Or locale? This method does not know anything about the origin of given localeVariantName. For example, this method is invoked during outdated cookie transformation that does not know a locale or a locale id.

package.json Outdated
@@ -123,6 +124,7 @@
"pretty": "2.0.0",
"puppeteer": "1.4.0",
"raw-loader": "0.5.1",
"regenerator-runtime": "^0.11.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Who uses generators?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried to mimic what babel-polyfill does here, looking for a solution. Deleting

@@ -26,7 +26,7 @@
<script id="oil-configuration" type="application/configuration">
{
"poi_group_name": "axelSpringerSe_01",
"timeout" : "2"
"timeout" : "3"
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for? It makes the test wait 1s longer!

}
require('core-js/modules/es6.object.assign');
Copy link
Contributor

Choose a reason for hiding this comment

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

No way we need all this polyfills!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have reduced them to the absolute necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

This is super strange:

oil.js 19.569 kB -> 19.43 kB it got smaller!!!
hub.js 11.239 kB -> 17.496 kB okay, this has the polyfills

.assert.containsText('html', 'Custom Purpose 1', 'Checking custom purpose title')
.pause(100)
.useCss()
// deactivate because opening the page in IE10 and 9 the custom purposes, only the e2e test fails
Copy link
Contributor

Choose a reason for hiding this comment

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

You should add a FIXME and the ticket number of the Jira Ticket you created.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@@ -21,7 +21,7 @@ module.exports = {
'OIL Layer should be auto-hidden after the correct amount of time': function (browser) {
browser
.pause(500)
.waitForElementVisible(OIL_LAYER, 500, false)
.waitForElementVisible(OIL_LAYER, 2000, false)
.pause(2000)
.useXpath().waitForElementNotPresent(OIL_LAYER, 500)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the timeout is 3s, how can this ever work?!

echo "- Testing $1 -"
echo "..."
echo "-------------------------"
echo "試)TESTING $1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Chinese?!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Japanese I know but probably the same character in Chinese, too.
試す【ためす】= to attempt; to test; to try out​
I've put it in as an eye catcher

@Waschnick Waschnick merged commit 6809bac into master Jun 22, 2018
@phogel phogel deleted the oil-190-browserstacks branch June 23, 2018 11:53
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.

4 participants