Last Comment Bug 730668 - Clean up test_placesTXN.js
: Clean up test_placesTXN.js
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: Firefox 14
Assigned To: Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
:
Mentors:
Depends on: 575955
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-26 04:49 PST by Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
Modified: 2012-03-17 17:32 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch v1 (60.40 KB, patch)
2012-03-07 12:51 PST, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
no flags Details | Diff | Splinter Review
Proposed patch v2 (59.82 KB, patch)
2012-03-15 05:58 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
mak77: review+
Details | Diff | Splinter Review
proposed patch v3 (60.79 KB, patch)
2012-03-15 16:02 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
mak77: review+
Details | Diff | Splinter Review
proposed patch v3.1 (60.80 KB, patch)
2012-03-15 16:16 PDT, Tetsuharu OHZEKI [:tetsuharu] [UTC+9]
mak77: review+
Details | Diff | Splinter Review

Description Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-02-26 04:49:36 PST
Clean up test_placesTXN.js.
Comment 1 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-03-07 12:51:17 PST
Created attachment 603828 [details] [diff] [review]
Proposed patch v1
Comment 2 Marco Bonardo [::mak] 2012-03-14 15:40:59 PDT
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] 2012-03-15 03:42:50 PDT
(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] 2012-03-15 03:54:06 PDT
(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] 2012-03-15 05:58:30 PDT
Created attachment 606177 [details] [diff] [review]
Proposed patch v2
Comment 6 Marco Bonardo [::mak] 2012-03-15 07:51:17 PDT
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] 2012-03-15 11:05:37 PDT
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 9 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2012-03-15 16:02:54 PDT
Created attachment 606387 [details] [diff] [review]
proposed 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] 2012-03-15 16:05:49 PDT
Comment on attachment 606387 [details] [diff] [review]
proposed 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] 2012-03-15 16:14:40 PDT
(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] 2012-03-15 16:16:26 PDT
Created attachment 606398 [details] [diff] [review]
proposed patch v3.1
Comment 13 Marco Bonardo [::mak] 2012-03-15 16:22:22 PDT
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] 2012-03-16 15:18:27 PDT
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] 2012-03-17 05:31:11 PDT
(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) 2012-03-17 17:32:50 PDT
https://hg.mozilla.org/mozilla-central/rev/79c121cac352

Note You need to log in before you can comment on or make changes to this bug.