Issue 2258303002: Add testing cases for safe_browsing_navigation_observer class.

Issue 2258303002: Add testing cases for safe_browsing_navigation_observer class. (Closed)

Created:
4 years, 4 months ago by Jialiu Lin

Modified:
3 years, 11 months ago

Reviewers:
Nathan Parker

CC:
chromium-reviews

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Add testing cases for safe_browsing_navigation_observer class. BUG=639467 Committed: https://crrev.com/bab0738dd276d46a0591b41a07796dbd3e7062ac Cr-Commit-Position: refs/heads/master@{#413314}

Patch Set 2 : address nparker's comments #

Created: 4 years, 4 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+128 lines, -0 lines)			Patch
A	chrome/test/data/safe_browsing/download_protection/navigation_observer/double_redirect.html	View	1	1 chunk	+8 lines, -0 lines	0 comments	Download
A	chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html	View	1	1 chunk	+105 lines, -0 lines	0 comments	Download
A	chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect.html	View		1 chunk	+8 lines, -0 lines	0 comments	Download
A	chrome/test/data/safe_browsing/download_protection/navigation_observer/safe_page.html	View		1 chunk	+7 lines, -0 lines	0 comments	Download

Messages

Total messages: 12 (5 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Jialiu Lin

Hi nparker@, This CL includes test cases only. navigation_observer class will be up soon. PTAL ...

4 years, 4 months ago (2016-08-19 21:37:05 UTC) #2

Nathan Parker

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html File chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html (right): https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html#newcode5 chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:5: var node = document.getElementById(name); I think some comments would ...

4 years, 4 months ago (2016-08-19 21:59:13 UTC) #5

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
File
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html
(right):

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:5:
var node = document.getElementById(name);
I think some comments would help make this code more maintainable.  Maybe a line
about how this is used (what's the entry point that the tests interact with?)
and what each function's intention is.

It might be easier to edit/maintain if the js code were in a .js file.  Not
sure.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:8:
evt.initMouseEvent("click", false, true, window, 0, 0, 0, 0, 0,
What does this do?

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:16:
function getParameterByName(name, url) {
What does this do?

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:29:
if (node!= null)
nit: space before !=

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:45:
tab.document.write('<META HTTP-EQUIV="refresh" content="0;
url=data:application/octet-stream;base64,a2poYWxrc2hkbGtoYXNka2xoYXNsa2RoYWxraGtoYWxza2hka2xzamFoZGxramhhc2xka2hhc2xrZGgKYXNrZGpoa2FzZGpoYWtzaGRrYXNoZGtoYXNrZGhhc2tkaGthc2hka2Foc2RraGFrc2hka2FzaGRraGFzCmFza2pkaGFrc2hkbSxjbmtzamFoZGtoYXNrZGhhc2tka2hrYXNkCjg3MzQ2ODEyNzQ2OGtqc2hka2FoZHNrZGhraApha3NqZGthc2Roa3NkaGthc2hka2FzaGtkaAohISomXkAqJl4qYWhpZGFzeWRpeWlhc1xcb1wKa2Fqc2Roa2FzaGRrYXNoZGsKYWtzamRoc2tkaAplbmQK">');
Is this (or others) expected to trigger a warning, or is it just exercising the
navigation?

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:51:
var errorize=function(e){console.log(e);};
nit: spaces aroun ='s.  And does this need to be an anonymous function? Having a
name would help explain its purpose.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
File
chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect1.html
(right):

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect1.html:1:
<html>
nit: The filename could be "double_redirect" if that's what this is doing.

Jialiu Lin

Thanks, nparker@! Your comments are all addressed. https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html File chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html (right): https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html#newcode5 chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:5: var node ...

4 years, 4 months ago (2016-08-19 23:12:18 UTC) #6

Thanks, nparker@! Your comments are all addressed.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
File
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html
(right):

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:5:
var node = document.getElementById(name);
On 2016/08/19 at 21:59:13, Nathan Parker wrote:
> I think some comments would help make this code more maintainable.  Maybe a
line about how this is used (what's the entry point that the tests interact
with?) and what each function's intention is.
> 
> It might be easier to edit/maintain if the js code were in a .js file.  Not
sure.
Since majority of these js functions are onclick functions, I feel it is easier
to put them in the same file as their corresponding clickable links. 
I'll add more comments to make it clearer.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:8:
evt.initMouseEvent("click", false, true, window, 0, 0, 0, 0, 0,
On 2016/08/19 at 21:59:13, Nathan Parker wrote:
> What does this do?

Simulate a ctrl + click event (a.k.a open link in a new tab).

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:16:
function getParameterByName(name, url) {
On 2016/08/19 at 21:59:13, Nathan Parker wrote:
> What does this do?

This function is to parse parameter in the URL (e.g.
..../navigation_observer_tests.html?test_case=direct_download&new_tab=1), which
used to identify which link to click and whether link is opened in new tab or
not.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:29:
if (node!= null)
On 2016/08/19 at 21:59:13, Nathan Parker wrote:
> nit: space before !=

done. Added space around all = or != or == in javascript part.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:45:
tab.document.write('<META HTTP-EQUIV="refresh" content="0;
url=data:application/octet-stream;base64,a2poYWxrc2hkbGtoYXNka2xoYXNsa2RoYWxraGtoYWxza2hka2xzamFoZGxramhhc2xka2hhc2xrZGgKYXNrZGpoa2FzZGpoYWtzaGRrYXNoZGtoYXNrZGhhc2tkaGthc2hka2Foc2RraGFrc2hka2FzaGRraGFzCmFza2pkaGFrc2hkbSxjbmtzamFoZGtoYXNrZGhhc2tka2hrYXNkCjg3MzQ2ODEyNzQ2OGtqc2hka2FoZHNrZGhraApha3NqZGthc2Roa3NkaGthc2hka2FzaGtkaAohISomXkAqJl4qYWhpZGFzeWRpeWlhc1xcb1wKa2Fqc2Roa2FzaGRrYXNoZGsKYWtzamRoc2tkaAplbmQK">');
On 2016/08/19 at 21:59:12, Nathan Parker wrote:
> Is this (or others) expected to trigger a warning, or is it just exercising
the navigation?
Just exercising navigation. No warning will be triggered.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/navigation_observer_tests.html:51:
var errorize=function(e){console.log(e);};
On 2016/08/19 at 21:59:13, Nathan Parker wrote:
> nit: spaces aroun ='s.  And does this need to be an anonymous function? Having
a name would help explain its purpose.

Oops, sorry, I copied this one from mattm's testing cases. Added function name
and improved its formatting.

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
File
chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect1.html
(right):

https://codereview.chromium.org/2258303002/diff/1/chrome/test/data/safe_brows...
chrome/test/data/safe_browsing/download_protection/navigation_observer/redirect1.html:1:
<html>
On 2016/08/19 at 21:59:13, Nathan Parker wrote:
> nit: The filename could be "double_redirect" if that's what this is doing.

double_redirect sounds much better.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2258303002/20001

4 years, 4 months ago (2016-08-19 23:29:43 UTC) #9

commit-bot: I haz the power

4 years, 4 months ago (2016-08-20 03:50:57 UTC) #12

Message was sent while issue was closed.

Patchset 2 (id:??) landed as
https://crrev.com/bab0738dd276d46a0591b41a07796dbd3e7062ac
Cr-Commit-Position: refs/heads/master@{#413314}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages