Adding HTML Terminal test case: 1. TestAddBookmark, 2. Login as guest

Adding HTML Terminal test case: 1. TestAddBookmark, 2. Login as guest


BUG=128108
TEST=This is a test.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=148675

[Nirnimesh, 2012-07-18 00:53:04]

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
File chrome/test/functional/chromeos_crosh.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:49: for _ in
range(self.GetBrowserWindowCount()):
This is not necessary since setUp() already does so.

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:58: # Add bookmark.
The rest of the test does not seem to depend on opening the crosh at all.
Either:
1. Do not open crosh at all. ie just create the bookmark, OR
2. Use use the new tab's url as the url in line 62

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:67: self.assertEqual(count+1,
bookmarks.NodeCount())
need space around +

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:73: class
CroshLoginTest(pyauto.PyUITest):
Please add this test to chromeos_login.py

This is because scheduling tests that involve login/logout requires special
mechanisms, which might break subsequent tests if it's kept here.

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:76: def setUp(self):
If you follow my previous comment, this will not be required.

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:83: 
remove blank line

[tturchetto, 2012-07-25 00:30:48]

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
File chrome/test/functional/chromeos_crosh.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:49: for _ in
range(self.GetBrowserWindowCount()):
On 2012/07/18 00:53:05, Nirnimesh wrote:
> This is not necessary since setUp() already does so.

Done.

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:58: # Add bookmark.
On 2012/07/18 00:53:05, Nirnimesh wrote:
> The rest of the test does not seem to depend on opening the crosh at all.
> Either:
> 1. Do not open crosh at all. ie just create the bookmark, OR
> 2. Use use the new tab's url as the url in line 62
Used #2 suggestion.
Done

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:67: self.assertEqual(count+1,
bookmarks.NodeCount())
On 2012/07/18 00:53:05, Nirnimesh wrote:
> need space around +

Done.

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:73: class
CroshLoginTest(pyauto.PyUITest):
On 2012/07/18 00:53:05, Nirnimesh wrote:
> Please add this test to chromeos_login.py
> 
> This is because scheduling tests that involve login/logout requires special
> mechanisms, which might break subsequent tests if it's kept here.

Done.

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:76: def setUp(self):
On 2012/07/18 00:53:05, Nirnimesh wrote:
> If you follow my previous comment, this will not be required.

I moved this test to chromeos_login.py, therefore keeping this setup. Test
passed.

https://chromiumcodereview.appspot.com/10778006/diff/1/chrome/test/functional...
chrome/test/functional/chromeos_crosh.py:83: 
On 2012/07/18 00:53:05, Nirnimesh wrote:
> remove blank line

Done.

[Nirnimesh, 2012-07-25 07:03:44]

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
File chrome/test/functional/chromeos_crosh.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_crosh.py:48: """Test bookmark crosh."""
Make this better. It's not clear what the test is.

Verify that we can bookmark a crosh URL

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_crosh.py:54: self.assertEqual('crosh',
self.GetActiveTabTitle())
49-54 is the same as 23-28. Refactor to a separate helper function.

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_crosh.py:68: self.assertTrue(url.spec() in
node['url'])
use assertEqual

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
File chrome/test/functional/chromeos_login.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:226: def setUp(self):
Do not create a separate class so that you can reuse the setUp in line 25.
Consequently, remove this setUp

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:232: """Test we can use crosh in guest
mode."""
Test we can use -> Verify

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:240: self.assertEqual(0,
self.GetBrowserWindowCount())
hereafter this test is very similar to the test in other file. Move common
portions to test_utils so that you can call from here.

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:262: self.Logout()
remove. Not necessary. Line 30 ensures that we get a new UI session for each
test.

[tturchetto, 2012-07-26 22:44:28]

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
File chrome/test/functional/chromeos_crosh.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_crosh.py:48: """Test bookmark crosh."""
On 2012/07/25 07:03:45, Nirnimesh wrote:
> Make this better. It's not clear what the test is.
> 
> Verify that we can bookmark a crosh URL

Done.

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_crosh.py:54: self.assertEqual('crosh',
self.GetActiveTabTitle())
On 2012/07/25 07:03:45, Nirnimesh wrote:
> 49-54 is the same as 23-28. Refactor to a separate helper function.

Done.

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_crosh.py:68: self.assertTrue(url.spec() in
node['url'])
On 2012/07/25 07:03:45, Nirnimesh wrote:
> use assertEqual

I am verifying condition, therefore assertTrue?

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
File chrome/test/functional/chromeos_login.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:226: def setUp(self):
On 2012/07/25 07:03:45, Nirnimesh wrote:
> Do not create a separate class so that you can reuse the setUp in line 25.
> Consequently, remove this setUp

Done.

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:232: """Test we can use crosh in guest
mode."""
On 2012/07/25 07:03:45, Nirnimesh wrote:
> Test we can use -> Verify

Done.

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:240: self.assertEqual(0,
self.GetBrowserWindowCount())
On 2012/07/25 07:03:45, Nirnimesh wrote:
> hereafter this test is very similar to the test in other file. Move common
> portions to test_utils so that you can call from here.

Done.

https://chromiumcodereview.appspot.com/10778006/diff/8001/chrome/test/functio...
chrome/test/functional/chromeos_login.py:262: self.Logout()
On 2012/07/25 07:03:45, Nirnimesh wrote:
> remove. Not necessary. Line 30 ensures that we get a new UI session for each
> test.

Done.

[Nirnimesh, 2012-07-26 23:09:02]

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
File chrome/test/functional/chromeos_crosh.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
chrome/test/functional/chromeos_crosh.py:59: self.assertTrue(url.spec() in
node['url'])
why use 'in'?

wouldn't assertTrue(url.spec(), node['url]) work?

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
File chrome/test/functional/test_utils.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
chrome/test/functional/test_utils.py:439: """ Verify that crosh is opened
correctly.
Remove space after """

Mention that this test opens crosh. There isn't much verification going on here.

Also, mention that it assumes all browser windows are closed. Remove all else.
Do not explain code.

[tturchetto, 2012-07-26 23:37:47]

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
File chrome/test/functional/chromeos_crosh.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
chrome/test/functional/chromeos_crosh.py:59: self.assertTrue(url.spec() in
node['url'])
On 2012/07/26 23:09:03, Nirnimesh wrote:
> why use 'in'?
> 
> wouldn't assertTrue(url.spec(), node['url]) work?

Done.

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
File chrome/test/functional/test_utils.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/13001/chrome/test/functi...
chrome/test/functional/test_utils.py:439: """ Verify that crosh is opened
correctly.
On 2012/07/26 23:09:03, Nirnimesh wrote:
> Remove space after """
> 
> Mention that this test opens crosh. There isn't much verification going on
here.
> 
> Also, mention that it assumes all browser windows are closed. Remove all else.
> Do not explain code.

Done.

[Nirnimesh, 2012-07-26 23:57:21]

LGTM % comments.

Please make sure the test runs successfully before committing.

https://chromiumcodereview.appspot.com/10778006/diff/16001/chrome/test/functi...
File chrome/test/functional/test_utils.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/16001/chrome/test/functi...
chrome/test/functional/test_utils.py:439: """ This test opens crosh.
You still have space after """

https://chromiumcodereview.appspot.com/10778006/diff/16001/chrome/test/functi...
chrome/test/functional/test_utils.py:441: This function assumes all browser
windos are closed.
Assumes that no browser windows are open

[tturchetto, 2012-07-27 00:35:11]

https://chromiumcodereview.appspot.com/10778006/diff/16001/chrome/test/functi...
File chrome/test/functional/test_utils.py (right):

https://chromiumcodereview.appspot.com/10778006/diff/16001/chrome/test/functi...
chrome/test/functional/test_utils.py:439: """ This test opens crosh.
On 2012/07/26 23:57:21, Nirnimesh wrote:
> You still have space after """

Done.

https://chromiumcodereview.appspot.com/10778006/diff/16001/chrome/test/functi...
chrome/test/functional/test_utils.py:441: This function assumes all browser
windos are closed.
On 2012/07/26 23:57:21, Nirnimesh wrote:
> Assumes that no browser windows are open

Done.