730668 - Clean up test_placesTXN.js

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Clean up test_placesTXN.js.

Comment 1

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Proposed patch v1

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 603828 [details] [diff] [review]
Proposed patch v1

Review of attachment 603828 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for late, some general comments below, most of them apply to many lines, though I only commented on the first instance.
Just the fact we don't have anymore to pay attention in variable names conflicts is awesome!

::: toolkit/components/places/tests/unit/test_placesTxn.js
@@ +127,5 @@
>  // index at which items should begin
> +let bmStartIndex = 0;
> +
> +// get bookmarks root index
> +let root = bmsvc.bookmarksMenuFolder;

please, use PlacesUtils.bookmarksMenuFolderId
generally when using roots we should always use the PlacesUtils lazy getters to avoid crossing xpcom.
The comment is wrong, since this is not an index.

@@ +131,5 @@
> +let root = bmsvc.bookmarksMenuFolder;
> +
> +// create test bookmark
> +function createBookmark(aURI, aTitle, aParentId, aIndex) {
> +  aIndex = !aIndex ? bmsvc.DEFAULT_INDEX : aIndex;

This would do the wrong thing if aIndex is 0, that is a valid case. you should check != null ? aIndex : default

@@ +133,5 @@
> +// create test bookmark
> +function createBookmark(aURI, aTitle, aParentId, aIndex) {
> +  aIndex = !aIndex ? bmsvc.DEFAULT_INDEX : aIndex;
> +  return bmsvc.insertBookmark(aParentId, aURI, aIndex, aTitle);
> +}

that said, this helper doesn't really look useful to me... can we get rid of it and just pass the right index value below?

@@ +138,5 @@
> +// create test folder
> +function createFolder (aTitle, aParentFolder, aIndex) {
> +  aIndex = !aIndex ? bmsvc.DEFAULT_INDEX : aIndex;
> +  return bmsvc.createFolder(aParentFolder, aTitle, aIndex);
> +}

ditto on both things

@@ +143,5 @@
> +
> +// create test separator
> +function createSeparator (aParentId, aIndex) {
> +  return bmsvc.insertSeparator(aParentId, aIndex);
> +}

this, as well, looks quite useless.

@@ +172,5 @@
> +  test_edit_postData();
> +  test_tagURI_untagURI();
> +  test_aggregate_removeItem_Txn();
> +  test_create_item_with_childTxn();
> +  test_create_folder_with_child_itemTxn();

rather than this, let's just call run_next_test() here, and below define tests as:

add_test(function name() {
  ...
  run_next_test();
});

this starts making them async, for future purposes.

@@ +174,5 @@
> +  test_aggregate_removeItem_Txn();
> +  test_create_item_with_childTxn();
> +  test_create_folder_with_child_itemTxn();
> +
> +  bmsvc.removeObserver(observer, false);

move this into a do_register_cleanup() function.

@@ +178,5 @@
> +  bmsvc.removeObserver(observer, false);
> +}
> +
> +// Test creating a folder with a description
> +function test_create_folder_with_description() {

please remove the useless comments before each test, the function name should be good enough to express what the function is doing, no need for cc comments. If a comments says something additional and actually useful, just move it inside the test function.

@@ +180,5 @@
> +
> +// Test creating a folder with a description
> +function test_create_folder_with_description() {
> +  const kTEST_FOLDERNAME = "Test creating a folder with a description";
> +  const kTEST_DESCRIPTION = "this is my test description";

nit: the style for constants is either kCamelCase or UPPER_CASE, not both. there are more later, though won't comment further on this

@@ +227,3 @@
>    // Create to Root
> +  let txn = new PlacesCreateBookmarkTransaction(testURI, root,
> +                                                bmStartIndex, "Test creating an item");

useless comment

@@ +227,4 @@
>    // Create to Root
> +  let txn = new PlacesCreateBookmarkTransaction(testURI, root,
> +                                                bmStartIndex, "Test creating an item");
> +  txnManager.doTransaction(txn); // Also testing doTransaction

useless comment

@@ +228,5 @@
> +  let txn = new PlacesCreateBookmarkTransaction(testURI, root,
> +                                                bmStartIndex, "Test creating an item");
> +  txnManager.doTransaction(txn); // Also testing doTransaction
> +
> +  let id = bmsvc.getBookmarkIdsForURI(testURI)[0];

nit: and useless newline (doTransaction can be just before related tests, as undo and redo are.
This happens later too, though won't comment further on it.

Comment 3

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #2)

Thank you for your review, Marco.

I'll remove some helper methods (createBookmark, createFolder, createSeparator).
These are created to replace transactions constructor as passing parameters is.
But they are no meanig now.

And I have some question:

> @@ +172,5 @@
> > +  test_edit_postData();
> > +  test_tagURI_untagURI();
> > +  test_aggregate_removeItem_Txn();
> > +  test_create_item_with_childTxn();
> > +  test_create_folder_with_child_itemTxn();
> 
> rather than this, let's just call run_next_test() here, and below define
> tests as:
> 
> add_test(function name() {
>   ...
>   run_next_test();
> });
> 
> this starts making them async, for future purposes.

Which of styles should I write ?:

function test_A () {...}

add_test(function name() {
  test_A();
  run_next_test();
});

OR:

add_test(function test_A() {
  ...
  run_next_test();
});

-----------------------------
> @@ +174,5 @@
> > +  test_aggregate_removeItem_Txn();
> > +  test_create_item_with_childTxn();
> > +  test_create_folder_with_child_itemTxn();
> > +
> > +  bmsvc.removeObserver(observer, false);
> 
> move this into a do_register_cleanup() function.

Where should I insert do_register_cleanup(),
the end of run_test? or the last add_test()?

I didn't understand the time when the callback of do_register_cleanup() is called. (https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests)

Comment 4

[Marco Bonardo [:mak]]

(In reply to OHZEKI Tetsuharu [:saneyuki_s] from comment #3)

> > add_test(function name() {
> >   ...
> >   run_next_test();
> > });
> > 
> > this starts making them async, for future purposes.

> Which of styles should I write ?:

> add_test(function test_A() {
>   ...
>   run_next_test();
> });

this one

> Where should I insert do_register_cleanup(),
> the end of run_test? or the last add_test()?

at any time, possibly just after you add the observer.

> I didn't understand the time when the callback of do_register_cleanup() is
> called. (https://developer.mozilla.org/en/Writing_xpcshell-based_unit_tests)

when the test finishes, all registered cleanup functions are executed by the harness, you can register them at any time.

Comment 5

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: Proposed patch v2

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 606177 [details] [diff] [review]
Proposed patch v2

Review of attachment 606177 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine! Please just get a trybuild with only xpcshell, to check everything is fine across platforms.
Thank you!

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 606177 [details] [diff] [review]
Proposed patch v2

Review of attachment 606177 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/tests/unit/test_placesTxn.js
@@ +133,4 @@
>  function run_test() {
> +  bmsvc.addObserver(observer, false);
> +  do_register_cleanup(function () {
> +    bmsvc.removeObserver(observer, false);

I forgot, the second argument here should not be needed

Comment 8

[Marco Bonardo [:mak]]

see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#667

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: ｐroposed patch v3

(In reply to Marco Bonardo [:mak] from comment #7)
> ::: toolkit/components/places/tests/unit/test_placesTxn.js
> @@ +133,4 @@
> >  function run_test() {
> > +  bmsvc.addObserver(observer, false);
> > +  do_register_cleanup(function () {
> > +    bmsvc.removeObserver(observer, false);
> 
> I forgot, the second argument here should not be needed

Remove the second argument.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 606387 [details] [diff] [review]
ｐroposed patch v3

no need for a further review, was just a nit (the method works regardless even passing more arguments).  We often do this error in the codebase with removeObserver!

Comment 11

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Marco Bonardo [:mak] from comment #10)

Oops! I made simple a mistake....
I'll attachment new patch. 

xpcshell test was all green across platforms.
(Thank you, Nakano-san.)
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=78ce91eb721f

The change of next patch from v2 is related to the removing second parameter of addObserver/removeObserver only.
So I think we can check-in next patch.

Comment 12

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Attachment: proposed patch v3.1

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 606398 [details] [diff] [review]
proposed patch v3.1

Review of attachment 606398 [details] [diff] [review]:
-----------------------------------------------------------------

I actually thought to check that, but then forgot, good catch btw.
I think in future we may want to indeed make that param optional.

Comment 14

[Daniel Holbert [:dholbert]]

Fixed a few instances of end-of-line whitespace & landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c121cac352

Comment 15

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> Fixed a few instances of end-of-line whitespace & landed:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/79c121cac352

Thank you for fixing them & landed.

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/79c121cac352