Automated tests fixes and refactoring #7951

NejcZdovc · 2017-03-29T08:15:49Z

Submitted a ticket for my issue if one did not already exist.
Used Github auto-closing keywords in the commit message.
Added/updated tests for this change (for new code or code which already has tests).
Ran git rebase -i to squash commits (if needed).

Resolves #7950
Closes #8061

Changed from

to

Auditors

@bsclifton @bbondy

Refactor

waitForExist('X, Y, true) -> waitForElementCount(X, 0)

.waitUntil(function () {
    return this.getText(X).then((val) => val === Y)
})

-> waitForTextValue(X, Y)

isExisting(X).then((isExisting) => isExisting === false) -> waitForElementCount(X, 0)

.waitUntil(function () {
        return this.getValue(X).then((val) => val === Y)
})

-> waitForInputText(X, Y)

waitForVisible(X, Y, true) -> waitForElementCount(X, 0)

Note

In this PR I also added BRAVE_TEST_COMMAND_LOGS=1 flag to Travis. This way we can see the problem easier and resolve it faster

DO NOT SQUASH COMMITS

Test Plan

npm run test

Details: brave#7951

luixxiul · 2017-03-30T03:04:11Z

test/contents/autofillTest.js

@@ -213,7 +213,7 @@ describe('Autofill', function () {
        .waitForTextValue('.creditCardPExpirationDate',
          (expMonth < 10 ? '0' + expMonth.toString() : expMonth.toString()) + '/' + expYear.toString())
    })
-    it('autofills the credit card', function * () {
+    it.skip('autofills the credit card', function * () {


Update of #5389 is necessary I think.

Thank you will do

Details: brave#7951

NejcZdovc · 2017-04-03T22:34:50Z

@darkdh Because of Standard I needed to add some brackets to this file app/browser/ads/adBlockUtil.js. Can you please double check if everything is still ok?

darkdh · 2017-04-03T22:41:46Z

app/browser/ads/adBlockUtil.js

+          siteHacks[firstPartyUrl.hostname] &&
+          siteHacks[firstPartyUrl.hostname].allowFirstPartyAdblockChecks
+        )
+      ) &&


I think this ) should be in L39 so that resourceType !== 'mainFrame' only affect isThirdPartyHost(firstPartyUrl.hostname || '', url.hostname)

Thank you. Please verify the change

lgtm now! Thanks for doing this 😉

bsclifton · 2017-04-03T23:06:47Z

@NejcZdovc ready for another rebase 😛

NejcZdovc · 2017-04-03T23:27:39Z

We have two tests that can be fixed easily because they can be reproduced, but I don't know how to fix it. It fails in tabHandles(). This are tests in bravery-components and misc-components.

cc @bbondy

Details: brave#7951

NejcZdovc · 2017-04-03T23:35:33Z

@bsclifton rebased

diracdeltas · 2017-04-04T18:50:05Z

js/stores/eventStore.js

@@ -54,7 +54,7 @@ const addPageView = (url, tabId) => {
    tab.isDestroyed() ||
    !tab.session.partition.startsWith('persist:')

-  if (url && isSourceAboutUrl(url) || isPrivate) {
+  if (url && (isSourceAboutUrl(url) || isPrivate)) {


i think this should be (url && isSourceAboutUrl(url)) || isPrivate, assuming the previous code was correct

Fixed, thank you

Details: brave#7951

diracdeltas · 2017-04-04T19:04:42Z

app/browser/ads/adBlockUtil.js

@@ -31,16 +31,27 @@ const mapFilterType = {
 const shouldDoAdBlockCheck = (resourceType, firstPartyUrl, url, shouldCheckMainFrame) =>


i thought this function was supposed to do the following instead:

firstPartyUrl.protocol && // By default first party hosts are allowed, but enable the check if a flag is specified in siteHacks ( shouldCheckMainFrame || ( ( resourceType !== 'mainFrame' && isThirdPartyHost(firstPartyUrl.hostname || '', url.hostname) ) || ( siteHacks[firstPartyUrl.hostname] && siteHacks[firstPartyUrl.hostname].allowFirstPartyAdblockChecks ) ) ) && // Only check http and https for now firstPartyUrl.protocol.startsWith('http') && // Only do adblock if the host isn't in the whitelist !whitelistHosts.find((whitelistHost) => whitelistHost === url.hostname || url.hostname.endsWith('.' + whitelistHost)) && // Make sure there's a valid resource type before trying to use adblock mapFilterType[resourceType] !== undefined

cc @darkdh

@diracdeltas I am not familiar with this code, so any feedback and help is more then welcome

for reference, here's what was in startAdBlocking prior to 85464b9:

const cancel = firstPartyUrl.protocol && (shouldCheckMainFrame || (details.resourceType !== 'mainFrame' && Filtering.isThirdPartyHost(firstPartyUrlHost, urlHost))) && firstPartyUrl.protocol.startsWith('http') && mapFilterType[details.resourceType] !== undefined && !whitelistHosts.includes(urlHost) && !urlHost.endsWith('.disqus.com') && adblock.matches(details.url, mapFilterType[details.resourceType], firstPartyUrl.host)

yes @diracdeltas is right.
Please do test that youtube videos do not give ads just to be sure though (logged in and out) and also do the test cases here:
fc24493#diff-1f92b9d4310a18761948d6ea1fbefc84

Code from @diracdeltas first comment was added

Test were refactored to prevent intermediate fails and optimization of the code List of changes: brave#7951

@bsclifton

Resolves brave#7950 Auditors: @bsclifton Test Plan: - `npm run test -- --grep="navigationBar tests"`

Test were refactored to prevent intermediate fails and optimization of the code List of changes: brave#7951

All test command has now log, so that we know which one is called We also have verbose mode in travis by default

Sites for about:bookmarks in now added as a batch Test Plan: - `npm run test -- --grep="bookmarks"`

cezaraugusto

Tests described on test plan all passed except for ~~navigationBar tests lockIcon shows insecure icon on an HTTP PDF but I think it's not related to PR~~. Addressed, works ++

Lint for standard@9.0.0 works as well. LGTM

This is a follow up of brave#7813 and resolves linter errors Resolves brave#8061

Now we get title from a toolbar and use this for test value Test Plan: - npm run test -- --grep="bookmark pdf"

NejcZdovc added the automated-tests label Mar 29, 2017

NejcZdovc added this to the 0.14.1 milestone Mar 29, 2017

NejcZdovc self-assigned this Mar 29, 2017

luixxiul added the PR/needs-QA-attention ☕ label Mar 29, 2017

luixxiul requested a review from bsclifton March 29, 2017 08:20

NejcZdovc force-pushed the hotfix/#7950-tests branch from 8a5d0dc to f6bcfb0 Compare March 29, 2017 10:29

NejcZdovc removed the PR/needs-QA-attention ☕ label Mar 29, 2017

NejcZdovc removed the request for review from bsclifton March 29, 2017 10:31

NejcZdovc added the PR/work-in-progress ⚒ label Mar 29, 2017

NejcZdovc changed the title ~~Automated tests fixes~~ Automated tests fixes and refactoring Mar 29, 2017

NejcZdovc force-pushed the hotfix/#7950-tests branch from f6bcfb0 to e42bf66 Compare March 29, 2017 12:39

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Mar 29, 2017

Test refactoring

e42bf66

Details: brave#7951

NejcZdovc force-pushed the hotfix/#7950-tests branch 2 times, most recently from 32e0228 to 06d7007 Compare March 29, 2017 21:12

luixxiul reviewed Mar 30, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#7950-tests branch from c89f00b to e257fd5 Compare March 30, 2017 13:48

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Mar 30, 2017

Test refactoring

740210a

Details: brave#7951

NejcZdovc force-pushed the hotfix/#7950-tests branch from 52a4838 to 81587d3 Compare March 30, 2017 15:28

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Mar 30, 2017

Test refactoring

51dcaf3

Details: brave#7951

NejcZdovc force-pushed the hotfix/#7950-tests branch from 53a4e29 to 5a24237 Compare April 3, 2017 21:49

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Apr 3, 2017

Test refactoring

2becedc

Details: brave#7951

NejcZdovc force-pushed the hotfix/#7950-tests branch from 5a24237 to 92ed4e3 Compare April 3, 2017 22:32

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Apr 3, 2017

Test refactoring

f61dc92

Details: brave#7951

NejcZdovc requested a review from darkdh April 3, 2017 22:34

darkdh reviewed Apr 3, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#7950-tests branch 3 times, most recently from d2c49da to bec610a Compare April 3, 2017 22:50

NejcZdovc force-pushed the hotfix/#7950-tests branch from bec610a to 1b04f89 Compare April 3, 2017 23:34

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Apr 3, 2017

Test refactoring

0d3f572

Details: brave#7951

NejcZdovc added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Apr 4, 2017

NejcZdovc requested review from bbondy and bsclifton April 4, 2017 09:33

NejcZdovc mentioned this pull request Apr 4, 2017

Fix Standard.js lint errors #8061

Closed

NejcZdovc force-pushed the hotfix/#7950-tests branch from 0dffa63 to 111e01d Compare April 4, 2017 12:04

diracdeltas reviewed Apr 4, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#7950-tests branch from 111e01d to 4e11205 Compare April 4, 2017 19:02

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Apr 4, 2017

Test refactoring

2c78646

Details: brave#7951

diracdeltas reviewed Apr 4, 2017

View reviewed changes

NejcZdovc force-pushed the hotfix/#7950-tests branch 3 times, most recently from 838c6da to 24ad5c4 Compare April 5, 2017 15:49

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this pull request Apr 5, 2017

Refactor of tests

395765b

Test were refactored to prevent intermediate fails and optimization of the code List of changes: brave#7951

NejcZdovc added 5 commits April 5, 2017 17:49

Navbar test fix

69b4367

Resolves brave#7950 Auditors: @bsclifton Test Plan: - `npm run test -- --grep="navigationBar tests"`

Refactor of tests

c77ebc4

Test were refactored to prevent intermediate fails and optimization of the code List of changes: brave#7951

Test fixes 1

90beee1

More logging and debugging in tests

8a0926d

All test command has now log, so that we know which one is called We also have verbose mode in travis by default

Fixes intermediate fails in bookmarks test

d046ad0

Sites for about:bookmarks in now added as a batch Test Plan: - `npm run test -- --grep="bookmarks"`

NejcZdovc force-pushed the hotfix/#7950-tests branch from 24ad5c4 to 3869152 Compare April 5, 2017 15:54

cezaraugusto approved these changes Apr 5, 2017

View reviewed changes

NejcZdovc added 2 commits April 5, 2017 18:00

Linter fixes

361753b

This is a follow up of brave#7813 and resolves linter errors Resolves brave#8061

Bookmark pdf location check fix

79e5f72

Now we get title from a toolbar and use this for test value Test Plan: - npm run test -- --grep="bookmark pdf"

NejcZdovc force-pushed the hotfix/#7950-tests branch from 3869152 to 79e5f72 Compare April 5, 2017 16:00

NejcZdovc merged commit 952790b into brave:master Apr 5, 2017

luixxiul removed the PR/needs-QA-attention ☕ label Apr 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automated tests fixes and refactoring #7951

Automated tests fixes and refactoring #7951

NejcZdovc commented Mar 29, 2017 •

edited

Loading

luixxiul Mar 30, 2017

NejcZdovc Mar 30, 2017

NejcZdovc commented Apr 3, 2017 •

edited

Loading

darkdh Apr 3, 2017

NejcZdovc Apr 3, 2017

darkdh Apr 3, 2017

bsclifton commented Apr 3, 2017

NejcZdovc commented Apr 3, 2017 •

edited

Loading

NejcZdovc commented Apr 3, 2017

diracdeltas Apr 4, 2017

NejcZdovc Apr 4, 2017

diracdeltas Apr 4, 2017 •

edited

Loading

NejcZdovc Apr 4, 2017

diracdeltas Apr 4, 2017

diracdeltas Apr 4, 2017

bbondy Apr 5, 2017

NejcZdovc Apr 5, 2017

cezaraugusto left a comment •

edited

Loading

		@@ -31,16 +31,27 @@ const mapFilterType = {
		const shouldDoAdBlockCheck = (resourceType, firstPartyUrl, url, shouldCheckMainFrame) =>

Automated tests fixes and refactoring #7951

Automated tests fixes and refactoring #7951

Conversation

NejcZdovc commented Mar 29, 2017 • edited Loading

Auditors

Refactor

Note

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton commented Apr 3, 2017

NejcZdovc commented Apr 3, 2017 • edited Loading

NejcZdovc commented Apr 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diracdeltas Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cezaraugusto left a comment • edited Loading

Choose a reason for hiding this comment

NejcZdovc commented Mar 29, 2017 •

edited

Loading

NejcZdovc commented Apr 3, 2017 •

edited

Loading

NejcZdovc commented Apr 3, 2017 •

edited

Loading

diracdeltas Apr 4, 2017 •

edited

Loading

cezaraugusto left a comment •

edited

Loading