568287 - Expand test for 'Copy Password' context menu item in Password Manager

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect)

Description

[Frank Yan (:fryn)]

Attachment: patch v1

Also replaces <popup /> with <menupopup /> in the actual code.

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 447563 [details] [diff] [review]
patch v1

Patch is fine, but please update since zpao just changed this test a bit in bug 568995.

Comment 3

[Frank Yan (:fryn)]

Attachment: patch v2 (now using SimpleTest.waitForClipboard)

Comment 4

[Frank Yan (:fryn)]

Attachment: patch v2.1 (now using SimpleTest.waitForClipboard)

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 449412 [details] [diff] [review]
patch v2.1 (now using SimpleTest.waitForClipboard)

>+            isMenuitemDisabled("true");

This feels very awkward.

It's would be cleaner to just have isMenuitemDisabled() return true or false, and move the is() checks into the caller. And flip the logic so it's isMenuitemEnabled (to prevent double-negatives). EG:

  is(isMenuitemEnabled(), true, "should be enabled")
  ...
  is(isMenuitemEnabled(), false, "should be disabled")


>+            selection.select(2);
>+            if (isMenuitemDisabled(""))
>+                menuitem.doCommand();
>+        }

The test shouldn't have conditional branches, this should just be:

  selection.select(2);
  is(isMenuItemEnabled(), true, "should be enabled");
  menuitem.doCommand().

If the menuitem is somehow actually disabled, the is() check will fail and it's ok to blow up after that.

Otherwise looks fine.

Comment 6

[Frank Yan (:fryn)]

Attachment: patch v2.2 (no mo' awkwardness)

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 449954 [details] [diff] [review]
patch v2.2 (no mo' awkwardness)

>+            return !menuitem.getAttribute("disabled");

Oh, I suppose that could be just !menuitem.disabled, but I don't care either way since it's just a test.

Comment 8

[Frank Yan (:fryn)]

(In reply to comment #7)
> (From update of attachment 449954 [details] [diff] [review])
> >+            return !menuitem.getAttribute("disabled");
> 
> Oh, I suppose that could be just !menuitem.disabled, but I don't care either
> way since it's just a test.

My reaction to reading this sentence:
OTL......... \o/

Comment 9

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Pushed http://hg.mozilla.org/mozilla-central/rev/0eed88315d8e (didn't bother with the !menuitem.disabled change)