Modifications to activity logging end to end tests. Reduces the number of window.open calls to make…

Modifications to activity logging end to end tests. Reduces the number of window.open calls to make the tests a lot faster.

BUG=256756, 245594
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=212061

[karenlees, 2013-07-10 20:18:29]

This is my first draft to speed up the tests.

I copied the incognito changes in for the moment.  When Michael's change is
submitted I can rebase and uncomment the incongito changes.

The change basically merges the functions that do something simple on a tab
updated into:
  executeApiCallsOnTabUpdated  and
  executeDOMChangesOnTabUpdated

[felt, 2013-07-10 21:44:28]

I'll re-read the code part after you rebase, I got a bit confused about which
changes were yours & which things were only temporarily commented out (vs caused
by the bug).

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/activity_log_private/README (right):

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/activity_log_private/README:14: (1) Add a
function to call the chrome api calls you want to test. If you need
re-wrap please

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/activity_log_private/README:24: - Calling
window.open in can use up to 3 secs per call in the debug
"in can use up" -- I think you a word :)

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/activity_log_private/README:29: -
executeDOMChangesOnTabUpdated
You asked me to check for clarity -- overall, I think this is clear.

[karenlees, 2013-07-10 23:55:46]

Fixed the readme file. I'll ping when I've done the rebase and uploaded again.

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
File chrome/test/data/extensions/api_test/activity_log_private/README (right):

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/activity_log_private/README:14: (1) Add a
function to call the chrome api calls you want to test. If you need
On 2013/07/10 21:44:28, felt wrote:
> re-wrap please

Done.

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/activity_log_private/README:24: - Calling
window.open in can use up to 3 secs per call in the debug
On 2013/07/10 21:44:28, felt wrote:
> "in can use up" -- I think you a word :)

Done.

https://codereview.chromium.org/19014003/diff/1/chrome/test/data/extensions/a...
chrome/test/data/extensions/api_test/activity_log_private/README:29: -
executeDOMChangesOnTabUpdated
On 2013/07/10 21:44:28, felt wrote:
> You asked me to check for clarity -- overall, I think this is clear.

Great. Thanks!

[karenlees, 2013-07-11 18:18:04]

In the end I copied the changes into a new git client as the merge after
rebasing was complicated.

I can delete this change in favor of https://codereview.chromium.org/18760004/

[mvrable, 2013-07-11 18:30:05]

A did a quick read-through...I haven't tried actually running it but the changes
seem reasonable to me.  Two minor comments only, and otherwise I'm happy.

https://codereview.chromium.org/19014003/diff/16001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/activity_log_private/test/test.js
(right):

https://codereview.chromium.org/19014003/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/activity_log_private/test/test.js:177:
func: function triggerApiCallsOnTabsUpdatedIncognito() {
This test case should presumably also be marked is_incognito: true.

https://codereview.chromium.org/19014003/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/activity_log_private/test/test.js:244:
'Non-incognito URL was anonymized');
Looks like just a formatting change here, and I think it changes it from
style-conforming to non-style-coforming (unless I don't understand the
JavaScript style).

[karenlees, 2013-07-11 19:00:46]

https://codereview.chromium.org/19014003/diff/16001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/activity_log_private/test/test.js
(right):

https://codereview.chromium.org/19014003/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/activity_log_private/test/test.js:177:
func: function triggerApiCallsOnTabsUpdatedIncognito() {
On 2013/07/11 18:30:05, mvrable wrote:
> This test case should presumably also be marked is_incognito: true.

Done.

https://codereview.chromium.org/19014003/diff/16001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/activity_log_private/test/test.js:244:
'Non-incognito URL was anonymized');
On 2013/07/11 18:30:05, mvrable wrote:
> Looks like just a formatting change here, and I think it changes it from
> style-conforming to non-style-coforming (unless I don't understand the
> JavaScript style).

Sorry, was unintentional change when I pasted in the method. Thanks for
spotting.

[felt, 2013-07-11 19:44:19]

one nit

https://codereview.chromium.org/19014003/diff/21001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js
(right):

https://codereview.chromium.org/19014003/diff/21001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:324:
'document.write("sent an XHR");';
I don't think the document.write is needed here anymore

[karenlees, 2013-07-11 21:25:21]

Fixed comments and a bracket in the wrong place.

https://codereview.chromium.org/19014003/diff/21001/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js
(right):

https://codereview.chromium.org/19014003/diff/21001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/activity_log_private/friend/reply.js:324:
'document.write("sent an XHR");';
On 2013/07/11 19:44:20, felt wrote:
> I don't think the document.write is needed here anymore

Done.

[felt, 2013-07-12 17:15:25]

lgtm, but you'll need Matt to review also because the commit queue is still
broken for new committers.

Ankur, can you please take a look at this this morning if you still have
concerns?

[karenlees, 2013-07-12 17:45:07]

On 2013/07/12 17:15:25, felt wrote:
> lgtm, but you'll need Matt to review also because the commit queue is still
> broken for new committers.
> 
> Ankur, can you please take a look at this this morning if you still have
> concerns?

Ankur - the dom change functions work in the same way as before by executing a
script on a new tab. I just merged a lot of the scripts into one, so only one
window.open is required test most of the dom logging. I didn't update functions
have more complex listeners on tab updated. Functions that don't open any new
tabs are also not as they are fast enough already. If you think any of the
scripts shouldn't be merged let me know.  The function called
"executeDOMChangesOnTabUpdated" is the main thing to look at.

[karenlees, 2013-07-12 17:56:54]

On 2013/07/12 17:45:07, karenlees wrote:
> On 2013/07/12 17:15:25, felt wrote:
> > lgtm, but you'll need Matt to review also because the commit queue is still
> > broken for new committers.
> > 
> > Ankur, can you please take a look at this this morning if you still have
> > concerns?
> 
> Ankur - the dom change functions work in the same way as before by executing a
> script on a new tab. I just merged a lot of the scripts into one, so only one
> window.open is required test most of the dom logging. I didn't update
functions
> have more complex listeners on tab updated. Functions that don't open any new
> tabs are also not as they are fast enough already. If you think any of the
> scripts shouldn't be merged let me know.  The function called
> "executeDOMChangesOnTabUpdated" is the main thing to look at.

The other thing I might not have explained is we now check the exact sequence of
api/dom calls logged. So if anything breaks it's easy to tell how far it got
through the script before it broke and which calls are not being logged. 
Previously the tests didn't do this, so I guess they needed to be separate to
tell which things was broken.

[Ankur Taly, 2013-07-12 17:56:54]

lgtm

[karenlees, 2013-07-12 18:05:06]

Hi Matt - 

Could you take a quick look at this.  Adrienne has already lgtm'd but the commit
queue is still broken for new committers so it won't accept her stamp of
approval.

The change minimizes the number of window.open calls to speed up the tests (see
chrome bug 245594).

Thanks!

[Matt Perry, 2013-07-15 18:53:16]

rubberstamp lgtm

[commit-bot: I haz the power, 2013-07-15 18:58:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/karenlees@chromium.org/19014003/589173...

[karenlees, 2013-07-15 21:52:00]

On 2013/07/15 18:58:25, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>
https://chromium-status.appspot.com/cq/karenlees%40chromium.org/19014003/5891...

Adrienne's fix to the blocked api calls
(https://chromiumcodereview.appspot.com/18769009) went in faster than I
anticipated :)   The cq tests failed because the blocked api calls are now being
logged (this cl added some logic to check blocked api calls so it failed on this
CL rather than Adrienne's).  I've rebased my client and added the expected
logging for the test.  It is now passing again on my machine and looks like the
blocked logging is definitely fixed :) 

Adrienne - can you take a quick look at the latest update and I'll attempt
another commit.

[felt, 2013-07-15 22:27:31]

On 2013/07/15 21:52:00, karenlees wrote:
> On 2013/07/15 18:58:25, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> >
>
https://chromium-status.appspot.com/cq/karenlees%2540chromium.org/19014003/58...
> 
> Adrienne's fix to the blocked api calls
> (https://chromiumcodereview.appspot.com/18769009) went in faster than I
> anticipated :)   The cq tests failed because the blocked api calls are now
being
> logged (this cl added some logic to check blocked api calls so it failed on
this
> CL rather than Adrienne's).  I've rebased my client and added the expected
> logging for the test.  It is now passing again on my machine and looks like
the
> blocked logging is definitely fixed :) 
> 
> Adrienne - can you take a quick look at the latest update and I'll attempt
> another commit.

lgtm, sorry that I forgot about this when landing my patch!

[commit-bot: I haz the power, 2013-07-15 23:10:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/karenlees@chromium.org/19014003/48002

[commit-bot: I haz the power, 2013-07-16 03:56:16]

Retried try job too often on linux_aura for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[felt, 2013-07-16 14:14:58]

On 2013/07/16 03:56:16, I haz the power (commit-bot) wrote:
> Retried try job too often on linux_aura for step(s) unit_tests
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

I'm seeing similar failures on linux_aura for another CL (issue 18360006), for
example:

http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/599...

Same stack trace for an ExtensionServiceTest. The failure doesn't look related
to either patch. I do see similar failures in the flakiness dashboard (like
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Aura/builds/8947),
but not very frequent -- whereas I am seeing a 100% failure rate for my patch.
>(

[karenlees, 2013-07-16 21:30:56]

On 2013/07/16 14:14:58, felt wrote:
> On 2013/07/16 03:56:16, I haz the power (commit-bot) wrote:
> > Retried try job too often on linux_aura for step(s) unit_tests
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
> 
> I'm seeing similar failures on linux_aura for another CL (issue 18360006), for
> example:
> 
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/599...
> 
> Same stack trace for an ExtensionServiceTest. The failure doesn't look related
> to either patch. I do see similar failures in the flakiness dashboard (like
>
http://build.chromium.org/p/chromium.webkit/builders/Linux%2520Aura/builds/8947),
> but not very frequent -- whereas I am seeing a 100% failure rate for my patch.
> >(

I've preemptively merged in your changes from 19338002 to check all the tests
pass, so I'll wait for that to get committed then rebase one more time before
attempting another commit.  

It turned out that the incognito mode URL cleaning is not working for DOM
logging, so I have disabled the checks on the URLs for those and added to TODO's
for mvrable to re-enable when it is fixed.

[commit-bot: I haz the power, 2013-07-17 11:12:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/karenlees@chromium.org/19014003/80001

[commit-bot: I haz the power, 2013-07-17 16:36:09]

Change committed as 212061