Clean up test_placesTXN.js

RESOLVED FIXED in Firefox 14

Status

()

Firefox
Bookmarks & History
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tetsuharu, Assigned: tetsuharu)

Tracking

Trunk
Firefox 14
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Clean up test_placesTXN.js.
(Assignee)

Updated

5 years ago
Depends on: 575955

Updated

5 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

5 years ago
Assignee: nobody → saneyuki.s.snyk
Created attachment 603828 [details] [diff] [review]
Proposed patch v1
Attachment #603828 - Flags: review?(mak77)
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.
Attachment #603828 - Flags: review?(mak77)
(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)
(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.
Created attachment 606177 [details] [diff] [review]
Proposed patch v2
Attachment #603828 - Attachment is obsolete: true
Attachment #606177 - Flags: review?(mak77)
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!
Attachment #606177 - Flags: review?(mak77) → review+
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
see http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsINavHistoryService.idl#667
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.
Attachment #606177 - Attachment is obsolete: true
Attachment #606387 - Flags: review?(mak77)
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!
Attachment #606387 - Flags: review?(mak77) → review+
(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.
Created attachment 606398 [details] [diff] [review]
proposed patch v3.1
Attachment #606387 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #606398 - Flags: review?(mak77)
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.
Attachment #606398 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Fixed a few instances of end-of-line whitespace & landed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/79c121cac352
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → Firefox 14
(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.
https://hg.mozilla.org/mozilla-central/rev/79c121cac352
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.