Closed Bug 1088041 Opened 10 years ago Closed 9 years ago

Enhance utils.getProperty/getEntity to only accept array and update the tests and libs using it

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Product:
Mozilla QA Graveyard
Component:
Mozmill Tests
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED WONTFIX

People

(Reporter: danisielm, Assigned: sandeep, Mentored)

References

(
URL
)

Details

(Whiteboard: [lib][lang=js][good first bug])

Attachments

(1 file, 7 obsolete files)

bugfix-1088041-d.patch
25.80 KB, patch
whimboo
: review-
Details | Diff | Splinter Review 
In bug 1088007 we are updating utils.getProperty to also accept array of URLs as the first parameter of this method.

What we need is to make the method as utils.getEntity() is. So only accept an array of urls. This will imply updating the method and all it's appearances in the test.
Hi, I'm happy to look at this.  Could you please provide some background on utils.getProperty()?  What does this method look like?
Flags: needinfo?(hskupin)
I would suggest you take a look at our documentation for Mozmill tests first, and how to run them:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests

Once you have the test repository cloned to your local disk, you will find this method in /libs/utils.js. The method itself is able to retrieve localized strings from the passed-in property files by specifying a property id. This works across all locales.
Assignee: nobody → sandeep
Status: NEW → ASSIGNED
Flags: needinfo?(hskupin)
Mentor: andrei → hskupin
OK thanks
Hi

I've installed mozmill using pip (using the installation instrutions given) -
the package is at /Library/Python/site-packages/mozmill and the script is in
/usr/local/bin, which is my PATH.

But when I call mozmill I receive the following message:
-bash: mozilla: command not found
-bash: cd: mozmill-env: No such file or directory

I have to specify the full path /usr/local/bin/mozmill.
Have cloned this repo on my machine using git, and created a new branch for this bug fix.  Looking at the function definition of getProperty() in utils.js (line 354) it has two parameters aUrls and aPropertyName, where aUrls is either a singleton or a proper array, and the line

   var urls = (typeof aUrls === "string") ? [aUrls] : aUrls;

which is identical to what is done in getEntity (same signature as getProperty()).  Where is the bug here or am I missing something?

Thanks
(In reply to Sandeep Murthy from comment #4)
> I've installed mozmill using pip (using the installation instrutions given) -
> the package is at /Library/Python/site-packages/mozmill and the script is in
> /usr/local/bin, which is my PATH.

Not sure which installation instructions you have looked at, but we don't require to use pip to get Mozmill and all tools installed. We have a preconfigured environment to make use of. Please have a look at the documentation here: https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#Installing_Mozmill. Also we propose to not globally install python packages but create a virtual environment (https://virtualenv.pypa.io/en/latest/)

> But when I call mozmill I receive the following message:
> -bash: mozilla: command not found

You run `mozilla` instead of `mozmill`? 

> -bash: cd: mozmill-env: No such file or directory

The mozmill-env is something different as I have explained above.

(In reply to Sandeep Murthy from comment #5)
> Have cloned this repo on my machine using git, and created a new branch for
> this bug fix.  Looking at the function definition of getProperty() in
> utils.js (line 354) it has two parameters aUrls and aPropertyName, where
> aUrls is either a singleton or a proper array, and the line
> 
>    var urls = (typeof aUrls === "string") ? [aUrls] : aUrls;
> 
> which is identical to what is done in getEntity (same signature as
> getProperty()).  Where is the bug here or am I missing something?

Good catch Sandeep! It's indeed true so we should update both of the methods. To ensure an array is used as parameter you can use assert.equals(X, Y, message). Have a look into other modules to exactly see how to use it.

And thanks for being interested to fix it!
Summary: Enhance utils.getProperty to only accept array and update the tests and libs using it → Enhance utils.getProperty/getEntity to only accept array and update the tests and libs using it
OK sorry, for installation instructions I used this page

https://developer.mozilla.org/en-US/docs/Mozilla/Projects/Mozmill

but I have found the correct page now

https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests

Do you have deadlines?  Should have this done fairly soon, I guess
a couple of days at most, not this evening though (I'm off for the
day :))
No worries if you need a couple of days. There is not such a hard deadline!
Hi

I'm looking at getProperty and the line

    var urls = (typeof aUrls === "string") ? [aUrls] : aUrls;

Here you are using the ternary op ===, which seems unnecessary - typeof always returns a string for any argument and "string" is a string literal.  I think == is sufficient here.

If you want to check whether aUrls is an array the simplest way seems to be use Array.isArray, like

     var urls = ( Array.isArray( aUrls ) ) ? aUrls : "not array"

This will not check whether each entry in the array is a string, but I guess you could also do that check if necessary.  Also, if aUrls is not an array I assume we throw an exception?  At the moment you just assume that if aUrls is not an array then it is a valid URL string.  Please let me know how you want to proceed.

Sandeep.
Using isArray() here seems to be fine and indeed better for this case. Also as you say it is not necessary to check if each entry is a string. The check for the array should be enough.

In case of the exception we do this via a call to assert.equal(), or here even assert.ok() would work fine. There are plenty of examples in the modules and tests, so just look around a bit.
Hi

I hope I haven't made any embarassing errors but having looked at the definition of the assert object here (https://developer.mozilla.org/en/docs/Mozilla/JavaScript_code_modules/Assert.jsm) and the bug details this is what I have:

/**
 * Returns the value of an individual property
 *
 * @param {string[]} aUrls
 *        Array of URLs (or URL) of the string bundle(s)
 * @param {string} aPropertyName
 *        The property to get the value of
 *
 * @returns {string} The value of the requested property
 */
function getProperty(aUrls, aPropertyName) {

  var urls = aUrls;
  var property = null;
  
  try {
    assert.equal( Array.isArray( urls ), "true", "Not an array - " + aUrls );  
    urls.some(aURL => {
      var bundle = Services.strings.createBundle(aURL);
      try {
        property = bundle.GetStringFromName(aPropertyName);
        return true;
      }
      catch (ex) { }
    });
  }
  catch (ex) { }
  
  if (property === null) {
    assert.fail("Unknown property - " + aPropertyName);
  }  
  
  return property;  
}

The  objective is to ensure that getProperty( aUrls, aPropertyName ) only accepts the first parameter aUrls if it is a proper array (hopefully of strings)- so 'var urls = aUrls;' stores aUrls in urls, visible to the whole function, we then start a try catch block which starts with

  'assert.equal( Array.isArray( urls ), "true", "Not an array - " + aUrls );'

So here we follow the assert.equal( <actual result>, <expected result>, <error message> ) format  and we know assert will throw an error if actual does not match expected, which we catch.  If urls is an array then Array.isArray( urls ) should return true and no exception is thrown, otherwise Array.isArray( urls ) return false and an error is thrown which we catch - is there any custom code required in the catch block like a custom error?

I hope this is OK, or is there something I am missing?  If OK I will make the change to getEntity as well.

Sandeep
(In reply to Sandeep Murthy from comment #11)
> I hope I haven't made any embarassing errors but having looked at the
> definition of the assert object here
> (https://developer.mozilla.org/en/docs/Mozilla/JavaScript_code_modules/
> Assert.jsm) and the bug details this is what I have:

Lets see then. :) So regarding this link reference it is not directly from us, because we have our own implementation of the assertion types. Please see our test repository at /lib/assertions.js.

> function getProperty(aUrls, aPropertyName) {
> 
>   var urls = aUrls;

Given that you do not modify the urls, there is no need to do this new assignment. You can still use aUrls across this method. It will always be accessible.

>   try {
>     assert.equal( Array.isArray( urls ), "true", "Not an array - " + aUrls
> );  

The return of isArray is a boolean, so the check should be against true. Here you make use of a string, which should fail. Also please move this line out of the try block, so you see the exception in your test. In your case it will be eaten and we fail with unknown property.

Otherwise it is hard to see the changes, given that you haven't uploaded an actual diff. I would propose that you squash any commits on your dev branch first and then run `git format-patch HEAD^` to export the changes as a diff file, which you can attach to this bug. Then follow https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#Reviewing.

Let me know if you have other specific questions in how to work with patches.
OK thanks, I've made the changes now to getProperty in utils.js - can I test this in my browser or locally before making a commit?  If OK then I will update getEntity in the same way, and then make the commit and create a patch from using https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests.
Scratch the last question, I just found the examples in https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests, so I should be OK.
I ran my first test for the update for getProperty using

  /usr/local/bin/mozmill -t ./lib/utils.js -b /Applications/Firefox.app

and I received an error

------

ERROR | Test Failure | {
  "exception": {
    "message": "exports is not defined", 
    "lineNumber": 557, 
    "name": "ReferenceError", 
    "fileName": "resource://mozmill/modules/frame.js -> file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/utils.js"
  }
}
TEST-UNEXPECTED-FAIL | /Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/utils.js | <TOP_LEVEL>
TEST-END | /Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/utils.js | finished in 0ms
RESULTS | Passed: 0
RESULTS | Failed: 1
RESULTS | Skipped: 0

------

In line 557 of utils.js there is this assignment

    exports.appInfo = appInfo;

that seems to be causing problem, where does exports come from?
Here is the diff for getProperty:

diff --git a/lib/utils.js b/lib/utils.js
index 979ab32..902e92b 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -352,25 +352,27 @@ function getEntity(aUrls, aEntityId) {
  * @returns {string} The value of the requested property
  */
 function getProperty(aUrls, aPropertyName) {
-  var urls = (typeof aUrls === "string") ? [aUrls] : aUrls;
-  var property = null;
 
-  urls.some(aURL => {
+  var property = null;
+  
+  assert.equal( Array.isArray( aUrls ), true, "Not an array - " + aUrls );  
+  try {
+    aUrls.some(aURL => {
     var bundle = Services.strings.createBundle(aURL);
     try {
       property = bundle.GetStringFromName(aPropertyName);
       return true;
     }
     catch (ex) { }
-  });
-
+  });  
+  catch (ex) { }
+  
   if (property === null) {
     assert.fail("Unknown property - " + aPropertyName);
-  }
-
-  return property;
+  }  
+  
+  return property;  
 }
Patch for updating getProperty and getEntity methods in lib/utils to accept only arrays of URLs.
Hi,

I'm updating the test files in lib/tests which use getProperty/getEntity methods in lib/utils.js.  I found only two files in which calls were made to these methods only a single URL string, which I fixed.

There are also several folders named tests also in firefox/, firefox/lib/, and in metrofirefox/ and metrofirefox/lib.

Should I update these as well?  Also, which tests should I run using mozmill or all of the tests in firefox and metrofirefox?
(In reply to Sandeep Murthy from comment #18)
> There are also several folders named tests also in firefox/, firefox/lib/,
> and in metrofirefox/ and metrofirefox/lib.
>
> Should I update these as well?  Also, which tests should I run using mozmill
> or all of the tests in firefox and metrofirefox?

Yes, you will have to check all of them, and apply the new calling convention if necessary. You will have to run each testrun type which files you have modified. As best use the `testrun_functional` (and others) command line. See the MDN document I posted earlier how to do it. Keep in mind that you will have to specify the --repository option, which points to your local checkout of the mozmill-tests repository.
Comment on attachment 8550722 [details] [diff] [review]
Patch for updating getProperty and getEntity methods in lib/utils to accept only arrays of URLs

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

::: diff.txt
@@ +1,3 @@
> +commit daa61523af55169bc1ee3a0765d8043efd5c267d
> +Author: Sandeep Murthy <s.murthy@mykolab.com>
> +Date:   Fri Jan 16 13:39:48 2015 +0000

Please do not include the actual diff file into the patch. It's not necessary. So please remove it for the next version.

::: lib/utils.js
@@ +318,5 @@
>   */
>  function getEntity(aUrls, aEntityId) {
> +  var aEntityValue = null
> +  // assertion for checking aUrls is an array 
> +  assert.equal( Array.isArray( aUrls ), true, "Not an array - " + aUrls );

Please check our coding styles for placing white-spaces:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style

Also you wont need the comment above, when the assertion message is clear enough. The message itself should be phrased in a positive manner, so what you exactly check here.

@@ +341,2 @@
>    }
> +  catch (ex) { }

Which part can raise an exception here? There was no need for a try/catch in the past, so I wonder about it.

@@ +343,4 @@
>  
> +  if (aEntityValue === null) {
> +    assert.ok(aEntityValue, arguments.callee.name + ": Entity - " + aEntityId + " has been found");
> +  }

There is no need to wrap this line inside an if condition. You can keep it as it was before.

@@ +362,3 @@
>    var property = null;
>    
> +  // assertion for checking aUrls is an array 

Please attach a complete diff and not a partial one. Given that you use git, please do a rebase before the format-patch command.
OK, I will remove the try-catch, but I think was then looking at a different version of assert.equal in another document on MDN, not assert.equal in your lib/assertions.js.

Sorry about the delay, I will do the changes again, run the test against the local repo (I have created a separate branch from the master just for the bugfix), and submit tomorrow, if not by Wed.
In firefox/lib/tests/testBaseInContentPage.js we have the line

  var cmdKey = browserWindow.getEntity("addons.commandkey");

where no aUrls is being passed to getEntity in utils.js.  In all cases like this, where only one argument is being passed, is this an error or do I leave it alone?
It seems browserWindow.getEntity("addons.commandkey") is not a reference to getEntity in utils.js, just checking.
Right, our ui modules are wrapping this method by internally holding the necessary property and dtd files. The tests don't have to care about which urls have to be used this way.
OK, I have checked ALL the files in the qa-mozmill-tests repo in which calls are made to lib.utils.{getEntity(),getProperty()}, and made the appropriate changes to ensure urls are passed as arrays.  These are

../metrofirefox/lib/ui/toolbars.js
../metrofirefox/lib/ui/flyoutPanel.js
../metrofirefox/lib/ui/tabs.js
../metrofirefox/lib/ui/toolbars.js

../metrofirefox/tests/functional/testPopups/testPopupsBlocked.js
../metrofirefox/tests/functional/testPopups/testAlwaysAllow.js
../metrofirefox/tests/functional/testPopups/testAllowOnce.js

../firefox/lib/tabview.js
../firefox/lib/search.js
../firefox/lib/sessionstore.js
../firefox/lib/toolbars.js
../firefox/lib/downloads.js

../firefox/tests/addons/ide@seleniumhq.org/lib/selenium.js

../firefox/tests/remote/testGeolocation/testAlwaysShareLocation.js
../firefox/tests/remote/testSecurity/testGreenLarry.js
../firefox/tests/remote/testSecurity/testMixedContentPage.js
../firefox/tests/remote/testGeolocation/testNeverShareLocation.js
../firefox/tests/remote/testPreferences/testRemoveAllCookies.js
../firefox/tests/remote/testSecurity/testSafeBrowsingNotificationBar.js
../firefox/tests/remote/testGeolocation/testShareLocation.js
../firefox/tests/remote/testSecurity/testSSLDisabledErrorPage.js
../firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
../firefox/tests/remote/testSecurity/testSubmitUnencryptedInfoWarning.js

../firefox/tests/functional/testTabbedBrowsing/testNewTab.js
../firefox/tests/functional/testPreferences/testDisableCookies.js
../firefox/tests/functional/testPreferences/testEnableCookies.js
../firefox/tests/functional/testSecurity/testGreyLarry.js
../firefox/tests/functional/testGeolocation/testNotNowShareLocation.js
../firefox/tests/functional/testPreferences/testPasswordNotSaved.js
../firefox/tests/functional/testPreferences/testPasswordSavedAndDeleted.js
../firefox/tests/functional/testAwesomeBar/testPasteLocationBar.js
../firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
../firefox/tests/functional/testAddons/testPreferencesMasterPassword.js
../firefox/tests/functional/testPreferences/testRemoveCookie.js

../firefox/tests/functional/testPreferences/testRestoreHomepageToDefault.js
../firefox/tests/functional/testGeolocation/testShareLocation.js
However I am having some issues with testrun_functional.  From the location of local repo (bugfix branch of the local master) the way I call testrun_functional is

testrun_functional --repository=. --report=http://mozmill-crowd.blargon7.com/db/ /Applications/Firefox.app/

But I receive some error messages, here is the output from the call above.mozversion INFO | application_buildid: 20150108202552
mozversion INFO | application_changeset: 32e36869f84a
mozversion INFO | application_display_name: Firefox
mozversion INFO | application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
mozversion INFO | application_name: Firefox
mozversion INFO | application_remotingname: firefox
mozversion INFO | application_repository: https://hg.mozilla.org/releases/mozilla-release
mozversion INFO | application_vendor: Mozilla
mozversion INFO | application_version: 35.0
mozversion INFO | platform_buildid: 20150108202552
mozversion INFO | platform_changeset: 32e36869f84a
mozversion INFO | platform_repository: https://hg.mozilla.org/releases/mozilla-release
mozversion INFO | platform_version: 35.0
*** Application: Firefox 35.0 (/Applications/Firefox.app/Contents/MacOS/firefox)
*** Platform: Mac OS X 10.10.1 64bit
*** Cloning test repository to '/var/folders/md/qy7y7r3d2zn7wfmcpvc2mtk80000gn/T/tmpectO1m.workspace/mozmill-tests'
abort: repository . not found!
*** Removing test repository '/var/folders/md/qy7y7r3d2zn7wfmcpvc2mtk80000gn/T/tmpectO1m.workspace/mozmill-tests'
Traceback (most recent call last):
  File "/Users/srm/Documents/sandeep/cst/mozilla/mozmill-env/python-lib/mozmill_automation/testrun.py", line 338, in run
    self.repository.clone(path)
  File "/Users/srm/Documents/sandeep/cst/mozilla/mozmill-env/python-lib/mozmill_automation/repository.py", line 75, in clone
    self._exec(['clone', self.url, self.path], True)
  File "/Users/srm/Documents/sandeep/cst/mozilla/mozmill-env/python-lib/mozmill_automation/repository.py", line 42, in _exec
    return process.check_output(command).strip()
  File "/Users/srm/Documents/sandeep/cst/mozilla/mozmill-env/python-lib/mozmill_automation/process.py", line 25, in check_output
    raise subprocess.CalledProcessError(retcode, cmd)
CalledProcessError: Command '['hg', 'clone', '.', '/var/folders/md/qy7y7r3d2zn7wfmcpvc2mtk80000gn/T/tmpectO1m.workspace/mozmill-tests', '--cwd', '/Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests']' returned non-zero exit status 255
Oh, right! Our automation scripts do not support git, and you have a git repository here. For ease of running you can also use the mozmill command directly. But then please also test with the Nightly version of Firefox, because that will be the branch your patch will land first.

When calling mozmill just supply -m and the path to the manifest file directly under firefox/tests/%testrun%/manifest.ini.
OK, but I also have errors relating to a missing "exports" entity when I try to run tests, using mozmill, directly against the changed files.  Here is an example for lib/tabview.js:

$ mozmill -t firefox/lib/tabview.js -b /Applications/Firefox.app

mozversion INFO | application_buildid: 20150108202552
mozversion INFO | application_changeset: 32e36869f84a
mozversion INFO | application_display_name: Firefox
mozversion INFO | application_id: {ec8030f7-c20a-464f-9b0e-13a3a9e97384}
mozversion INFO | application_name: Firefox
mozversion INFO | application_remotingname: firefox
mozversion INFO | application_repository: https://hg.mozilla.org/releases/mozilla-release
mozversion INFO | application_vendor: Mozilla
mozversion INFO | application_version: 35.0
mozversion INFO | platform_buildid: 20150108202552
mozversion INFO | platform_changeset: 32e36869f84a
mozversion INFO | platform_repository: https://hg.mozilla.org/releases/mozilla-release
mozversion INFO | platform_version: 35.0
ERROR | Test Failure | {
  "exception": {
    "message": "exports is not defined", 
    "lineNumber": 631, 
    "name": "ReferenceError", 
    "fileName": "resource://mozmill/modules/frame.js -> file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tabview.js"
  }
}
TEST-UNEXPECTED-FAIL | /Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tabview.js | <TOP_LEVEL>
TEST-END | /Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tabview.js | finished in 0ms

###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID

###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID


###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xAA0001,name=PTexture::Msg___delete__) Route error: message sent to unknown actor ID
RESULTS | Passed: 0
RESULTS | Failed: 1
RESULTS | Skipped: 0
OK, I forgot the manifest file, sorry. :)
(1) Should I expect all tests to be passed for one of these manifest tests, and (2) should I save the output for each test to keep in the repo?  I just ran one and I have

RESULTS | Passed: 6
RESULTS | Failed: 15
RESULTS | Skipped: 3

errors are in files not changed by me, such as mozmill/stdlib/EventUtils.js.
(In reply to Sandeep Murthy from comment #28)
> $ mozmill -t firefox/lib/tabview.js -b /Applications/Firefox.app

This is not a test but a module You cannot run this. Each test starts with the test_ prefix in the filename. To test this module you should call the appropriate unit test instead which is under firefox/lib/tests.
Attached patch Bugfix-1088041.patch (obsolete) — — Splinter Review 
Proposed patch for bug #1088041
Attachment #8551801 - Flags: feedback?(hskupin)
Have submitted the patch (conversion to hg from git using git-patch-to-hg-patch, and before that, rebasing bugfix branch changes from local master which was updated from the GitHub repo).
Comment on attachment 8550722 [details] [diff] [review]
Patch for updating getProperty and getEntity methods in lib/utils to accept only arrays of URLs

Marking first patch as obsolete, given that a newer version has been uploaded. FYI you can do it yourself when uploading attachments, by marking old attachments as obsolete.
Attachment #8550722 - Attachment is obsolete: true
Comment on attachment 8551801 [details] [diff] [review]
Bugfix-1088041.patch

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

The patch you uploaded here is not a full patch but as it looks like only contains the changes from the last commit. Please make sure to rebase all the commits on your developer branch before running git format-patch.

::: firefox/tests/l10n/manifest.ini
@@ +3,5 @@
>  
>  [include:testAccessKeys/manifest.ini]
>  [include:testCropped/manifest.ini]
> +
> +

Please revert the changes in this file. It's not related.
Attachment #8551801 - Flags: feedback?(hskupin) → feedback-
The change to firefox/tests/l10n/manifest.ini was an accidental keystroke which I removed, but became part of the final commit.  I used git checkout for this file only, and commited the old version.

There are now five commits, but the first two were incomplete commits.  Can I just rebase from the first of the last three commits?
I am sorry but I am afraid things have gone wrong here.

There is now merge conflict and a patch failure when I try to do a rebase.  Why the last patch was not satisfactory?  According to the documentation rebase works by "going to the common ancestor of the two branches (the one you’re on and the one you’re rebasing onto), getting the diff introduced by each commit of the branch you’re on, saving those diffs to temporary files, resetting the current branch to the same commit as the branch you are rebasing onto, and finally applying each change in turn."

So rebase will apply all my commits in the working branch to the local master in a linear sequence, so isn't this the same thing as rebasing each commit I have done?

Please advise, because I don't think I can work with this branch anymore. Maybe I have to delete it, create a new one from the local master, do the changes again, and the patch.  That will take more days.

        Attachment #8551801 -
        Attachment is obsolete: true

        Attachment #8552475 -
        Flags: feedback?(hskupin)

    
    

    
  
OK, I did another patch, it includes all the local commits.  Hopefully this is satisfactory! :)

    
    

    
  
Sandeep, I think you have uploaded an invalid patch.

    
    

    
  
It's a compressed patch file.  The original was 16 MB.

    
    

    
  
Could you please explain why?  I am following the instructions here at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#Reviewing for rebasing againt latest changes to the remote master:

    git checkout master
    git pull origin master
    git checkout %work_branch%
    git rebase master

    
    

    
  
Comment on attachment 8552475 [details] [diff] [review]
Patch for bug #1088041 - includes all local commits

This link you posted is for using Mercurial. In your git steps you are not running the rebase with the -i option to actually squash all of your commits into a single one. Only after that format-patch will work, otherwise you only have the latest commit as part of the patch.

Patches should always be uploaded in text format. Not sure why you have such a large patch, but you certainly don't want to include all the commit on this repository. I will mark it as obsolete. If you have further questions, it will be better if you could join us on IRC so we can directly give instructions.

        Attachment #8552475 -
        Attachment is obsolete: true

        Attachment #8552475 -
        Flags: feedback?(hskupin)

    
    

    
  
The instructions I used were from the section on Git, not Mercurial.  It is the subsection with the title 'Rebase against latest changes to mozmill-tests' under Git.

I will try this one more time (rebase -i HEAD~x etc., I did try it before but it is reporting a problem with one of my previous commits), if it does work then I will join you on IRC.  It would be helpful if you have a specific time in mind, or generally any time.

    
    

    
  
Hi

I can't use IRC right now, but when I do a rebase from the last commit after which I want to start from (124be629c635a3946de2c9a75fdc94345ace9a7c) then I do git rebase -i 124be629c635a3946de2c9a75fdc94345ace9a7c,
and squash all the recent commits except for the very first I made on Jan. 19 (3fe14dcfa776053dcf932937242a582625c409c7), which I leave as pick.

The output is

error: could not apply 3fe14dc... Bug 1088041 - Changes to ensure lib.utils.{getEntity(),getProperty()} are called with arrays of URLs. r=hskupin

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".
Could not apply 3fe14dcfa776053dcf932937242a582625c409c7... Bug 1088041 - Changes to ensure lib.utils.{getEntity(),getProperty()} are called with arrays of URLs. r=hskupin

I don't why it can't apply this commit, or maybe I need to improve my Git knowledge, but this is a bit frustrating and not according to my plan.

Please advise.

    
    

    
  
The best in your situation might be to run a `git diff master > patch`, create a new branch, and import this patch. You will have a clean branch then for generating the patch.

    
    

    
  
This is not working either - I used git apply --check and the messages indicate there is some problem with the index on the branch I created for the bug fix.

Let me do the changes again, it will actually be quicker and safer.  Before making the patch I will request exact instructions on how to squash the commits into one (I would say your documentation on https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#Reviewing could include such instructions, because it is not exactly a simple procedure).

    
    

    
  
Thanks Sandeep. I updated the docs to make it clearer. Please have a look, if that works for you now.

    
    

    
  
Yes I see your updated instructions.

I've now redone all the original changes to the lib/ and firefox/tests files.   Before running the tests using the manifest files, please advise whether the output should be saved somewhere, or what if the output shows some tests failing.  I'm not clear about that, so please advise.

    
    

    
  
I have run all the tests now - some report failures, though not related to my changes.  I have saved the output of each test to a text file.

Please advise on what I should do with these, or should I now start the rebase producedure for all the commits I've done, and then produce the patch?

Ideally I would like to have this done today, but I need to know what to do about the test failures (I can email the files to you)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug-1088041.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Patch for bug #1088041.

        Attachment #8553731 -
        Flags: feedback?(hskupin)

    
    

    
  
Hi

I've uploaded the patch using the instructions given.  There were no problems, and the patch shows only one commit for all the changes.

You haven't said about what to do in relation to the test results - I saved each major set of test results, i.e. functional, in a text file.  I think the documentation https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests#Reviewing could be improved a bit more because if you are expected to run tests, after making changes in your working branch but before commits, then there is no information about what to do in case of test failures which show problems with files you have not changed.

    
    

    
  
Comment on attachment 8553731 [details] [diff] [review]
bug-1088041.patch

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

Hi Sandeep! That is a good start for the first complete patch! I like it. I looked through and found a couple of places where updates are necessary. Please check all my inline comments. Also please ensure to update testUtils.js so it reflects the changed API. Thanks!

::: firefox/lib/tabview.js
@@ +568,5 @@
>              case "title":
>                // If no title is given the default name is used
>                if (!value) {
> +                var tabviewPropsDtds = ["chrome://browser/locale/tabview.properties"];
> +                value = utils.getProperty(tabviewPropsDtds,

Just name the new variable `properties`.

::: firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
@@ +113,4 @@
>  
>    for (var i = 0; i < pluginNames.length; i++) {
>      if (pluginNames[i].textContent === aPluginName) {
> +      var dtds = "chrome://browser/locale/browser.properties";

Those are properties and not DTDs, so rename this variable to properties too. Same for all the other cases in this patch.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +119,4 @@
>    // Check the Owner label for "This web site does not supply ownership information."
>    var webIDOwnerLabel = new elementslib.ID(aController.window.document,
>                                             "security-identity-owner-value");
> +  var dtds = "chrome://browser/locale/pageInfo.properties";                                         

nit: Please get rid of those trailing white-spaces.

::: firefox/tests/remote/testSecurity/testSSLStatusAfterRestart.js
@@ +182,4 @@
>    // Check the Owner for "This web site does not supply ownership information."
>    var ownerElem = findElement.ID(aController.window.document,
>                                   "security-identity-owner-value");
> +  var dtds = "chrome://browser/locale/pageInfo.properties"                               

nit: same trailing white-spaces here.

::: lib/utils.js
@@ +317,4 @@
>   * @type string
>   */
>  function getEntity(aUrls, aEntityId) {
> +  var aEntityValue = null

Looks to be a left-over from a former change you did. Please remove it.

@@ +321,2 @@
>  
> +  assert.equal(Array.isArray(aUrls), true, "Checking aUrls is an array");

Please make use of assert.ok() here. It expects a boolean as result and will assert in case of false. For the comment please add a clean statement like "List of DTD URLs has been specified".

@@ +321,3 @@
>  
> +  assert.equal(Array.isArray(aUrls), true, "Checking aUrls is an array");
> +  

nit: unnecessary white-spaces. Maybe you want to enable your editor to show white-spaces and trim them automatically on save.

@@ +324,2 @@
>    // Add xhtml11.dtd to prevent missing entity errors with XHTML files
> +  aUrls.push("resource:///res/dtd/xhtml11.dtd");

As good practice you should not modify parameters of functions. So it would be better to initialize `urls` with that DTD URL at the beginning of the method and push the other urls to it.

@@ +330,2 @@
>      extEntities += '<!ENTITY % dtd' + i + ' SYSTEM "' +
> +                   aUrls[i] + '">%dtd' + i + ';';

This would then have to be reverted to `urls`

@@ +357,3 @@
>    var property = null;
>  
> +  assert.equal(Array.isArray(aUrls), true, "Checking aUrls is an array");  

Please apply the same as for getEntity here and fix the trailing white-space.

        Attachment #8553731 -
        Flags: feedback?(hskupin) → feedback+

    
    

    
  
Hi,

thanks for the feedback.  I'm assuming these changes should be rolled up into one single commit (using rebasing like before) and a new patch created?

Sandeep

    
    

    
  
That's totally correct, yes!

    
    

    
  
OK, could you please clarify what is to be about test failures (when I run mozmill -b /Applications/FirefoxNightly.app -m <manifest file>) and the test output?

At the moment just for my own record, for each manifest file (e.g. for functional) I redirect the output to a text file in a location outside the repo.  Do you need this output to be attached in some way?

I am only testing against FirefoxNightly.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug-1088041-srm.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Patch for the latest requirements (27/01/2015).  (Separate set of changes from the previous patch, so that is not marked as obsolete.)

        Attachment #8555366 -
        Flags: feedback?(hskupin)

    
    

    
  
(In reply to Sandeep Murthy from comment #56)
> OK, could you please clarify what is to be about test failures (when I run
> mozmill -b /Applications/FirefoxNightly.app -m <manifest file>) and the test
> output?

Important is the final output at the end. If all tests are passing or are skipped, its fine. But if you have failures, we might wanna check what went wrong. You can see details of those failures when you scroll through the log or search for "error".

> At the moment just for my own record, for each manifest file (e.g. for
> functional) I redirect the output to a text file in a location outside the
> repo.  Do you need this output to be attached in some way?

Best would be to install the mozmill-automation package from pypi. It offers all the different testrun_* commands in the shell to execute. With --repository a hg repository could be specified for the tests, and --report would send the results to our dashboard. See the MDN document for details. But sadly you are using git for your work locally. Maybe you can also make up a hg repo in this same folder, so it can then be used with the testrun scripts and the --repository parameter.

If that is too complicated just paste the final results with mozmill, and if you see failures also their details. Thanks.

    
    

    
  
I could set up Mercurial using the instructions given (https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests) and then follow the test procedure for that, but my next free time is on Friday.  So I will do this then.

    
    

    
  
Comment on attachment 8555366 [details] [diff] [review]
bug-1088041-srm.patch

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

This looks good to me, but please do not forget to update the testUtils.js file for this change.

        Attachment #8555366 -
        Flags: feedback?(hskupin) → feedback+

    
    

    
  
Hi

Do you mean to this line (#50, /lib/tests/testUtils.js)

var prop = utils.getProperty(TEST_DATA.properties.urls[0], TEST_DATA.properties.ids[0]);

We should be passing the first argument as an array, I guess.  All other calls to getProperty/getEntity in utils.js are using proper arrays which are defined in TEST_DATA.

Sandeep

    
    

    
  
That's totally correct, yes.

    
    

    
  
OK, is a patch required for this as well?

I will post the test results soon.

    
    

    
  
Please always update the patch as required, and mark the old one obsolete.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug-1088041-d.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Updated second patch (change to testUtils.js to ensure getEntity/getProperty in lib/utils.js are called with proper URL arrays).

        Attachment #8555366 -
        Attachment is obsolete: true

        Attachment #8556064 -
        Flags: feedback?(hskupin)

    
    

    
  
Logically, the first and second patches contain some changes which are unrelated, that's why I did not mark the first patch as obsolote.  Hope this is OK.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug-1088041-d.patch (obsolete)
        — 
          — Splinter Review
      

    


  
Updated patch (one missing URL array change has now been fixed).

        Attachment #8556064 -
        Attachment is obsolete: true

        Attachment #8556064 -
        Flags: feedback?(hskupin)

        Attachment #8556101 -
        Flags: feedback?(hskupin)

        Attachment #8553731 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8556101 [details] [diff] [review]
bug-1088041-d.patch

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

This patch is not a complete one but only contains the latest changes again. Please make sure that you really create a patch between your development branch and master by rebasing all your commits correctly.

        Attachment #8556101 -
        Flags: feedback?(hskupin) → feedback-

    
    

    
  
Sorry, please find the updated patch which now combines the two earlier patches.  On my system it appears to contain changes in all commits made since I first started working on this bug.

    
    

    
  


  

        Attachment #8556101 -
        Attachment is obsolete: true

        Attachment #8556446 -
        Flags: feedback?(hskupin)

    
    

    
  
lib-tests.txt

[1;34mTEST-START[0m | testBaseWindow.js | setupModule
[1;34mTEST-START[0m | testBaseWindow.js | testBaseWindow
[1;32mTEST-PASS[0m | testBaseWindow.js | testBaseWindow
[1;34mTEST-START[0m | testBaseWindow.js | teardownModule
TEST-END | testBaseWindow.js | finished in 2848ms
[1;34mTEST-START[0m | testCommonDialog.js | setupModule
[1;34mTEST-START[0m | testCommonDialog.js | testCommonDialog
[1;32mTEST-PASS[0m | testCommonDialog.js | testCommonDialog
[1;34mTEST-START[0m | testCommonDialog.js | teardownModule
TEST-END | testCommonDialog.js | finished in 326ms
[1;34mTEST-START[0m | testFormData.js | setupModule
[1;34mTEST-START[0m | testFormData.js | testFormData
[1;32mTEST-PASS[0m | testFormData.js | testFormData
[1;34mTEST-START[0m | testFormData.js | teardownModule
TEST-END | testFormData.js | finished in 2967ms
[1;34mTEST-START[0m | testOpenMultipleWindows.js | setupModule
[1;34mTEST-START[0m | testOpenMultipleWindows.js | testOpenMultipleWindows
[1;32mTEST-PASS[0m | testOpenMultipleWindows.js | testOpenMultipleWindows
[1;34mTEST-START[0m | testOpenMultipleWindows.js | teardownModule
TEST-END | testOpenMultipleWindows.js | finished in 1167ms
[1;34mTEST-START[0m | testPerformance.js | testPerfTracer
==================================
Performance Trace (testPerfTracer)
==================================
Thu, 29 Jan 2015 18:43:56 GMT | -1 | 303435776 | Checkpoint 0
Thu, 29 Jan 2015 18:43:57 GMT | -1 | 273932288 | Checkpoint 1
Thu, 29 Jan 2015 18:43:58 GMT | -1 | 278441984 | Checkpoint 2
Thu, 29 Jan 2015 18:43:59 GMT | -1 | 270376960 | Checkpoint 3
Thu, 29 Jan 2015 18:44:00 GMT | -1 | 270483456 | Checkpoint 4
Thu, 29 Jan 2015 18:44:01 GMT | -1 | 270483456 | Checkpoint 5
Thu, 29 Jan 2015 18:44:02 GMT | -1 | 269295616 | Checkpoint 6
Thu, 29 Jan 2015 18:44:03 GMT | -1 | 269332480 | Checkpoint 7
Thu, 29 Jan 2015 18:44:04 GMT | -1 | 269303808 | Checkpoint 8
Thu, 29 Jan 2015 18:44:05 GMT | -1 | 269316096 | Checkpoint 9
[1;32mTEST-PASS[0m | testPerformance.js | testPerfTracer
TEST-END | testPerformance.js | finished in 10081ms
[1;34mTEST-START[0m | testUtils.js | testUtils
[1;32mTEST-PASS[0m | testUtils.js | testUtils
TEST-END | testUtils.js | finished in 35ms
[1;34mTEST-START[0m | test_assertions.js | setupModule
[1;34mTEST-START[0m | test_assertions.js | testExpect
[1;32mTEST-PASS[0m | test_assertions.js | testExpect
[1;34mTEST-START[0m | test_assertions.js | testAssert
[1;32mTEST-PASS[0m | test_assertions.js | testAssert
[1;34mTEST-START[0m | test_assertions.js | teardownModule
TEST-END | test_assertions.js | finished in 10113ms
[1;34mTEST-START[0m | test_files.js | testReadFile
[1;32mTEST-PASS[0m | test_files.js | testReadFile
[1;34mTEST-START[0m | test_files.js | testRemoveFile
[1;32mTEST-PASS[0m | test_files.js | testRemoveFile
[1;34mTEST-START[0m | test_files.js | testProfileLocation
[1;32mTEST-PASS[0m | test_files.js | testProfileLocation
[1;34mTEST-START[0m | test_files.js | testWriteFile
[1;32mTEST-PASS[0m | test_files.js | testWriteFile
TEST-END | test_files.js | finished in 115ms
[1;34mTEST-START[0m | test_nodeCollector.js | setupModule
[1;34mTEST-START[0m | test_nodeCollector.js | testNodeCollector
[1;32mTEST-PASS[0m | test_nodeCollector.js | testNodeCollector
TEST-END | test_nodeCollector.js | finished in 15ms
RESULTS | Passed: 13
RESULTS | Failed: 0
RESULTS | Skipped: 0

    
    

    
  
firefox-lib-tests.txt

[1;34mTEST-START[0m | testAboutAccountsPage.js | setupModule
[1;34mTEST-START[0m | testAboutAccountsPage.js | testAboutAccounts
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Tab has been opened", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testAboutAccountsPage.js | testAboutAccounts
[1;34mTEST-START[0m | testAboutAccountsPage.js | teardownModule
TEST-END | testAboutAccountsPage.js | finished in 7443ms
[1;34mTEST-START[0m | testAboutPreferences.js | setupModule
[1;34mTEST-START[0m | testAboutPreferences.js | testPreferences
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Tab has been opened", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testAboutPreferences.js | testPreferences
[1;34mTEST-START[0m | testAboutPreferences.js | teardownModule
TEST-END | testAboutPreferences.js | finished in 6481ms
[1;34mTEST-START[0m | testAddonsBlocklist.js | setupModule
[1;34mTEST-START[0m | testAddonsBlocklist.js | testBlocklistAPI
error: An exception occurred.
Traceback (most recent call last):
  File "resource://jsbridge/modules/Sockets.jsm:40", line 11, in Sockets.Client.prototype.onMessage/event.notif
  File "resource://jsbridge/modules/Server.jsm:32", line 5, in Server.Session/
  File "resource://gre/components/multiprocessShims.js:130", line 52, in AddonInterpositionService.prototype.interpose/desc.valu
  File "resource://gre/modules/RemoteAddonsParent.jsm:688", line 60, in ComponentsUtilsInterposition.methods.evalInSandbo
  File "resource://gre/modules/RemoteAddonsParent.jsm:688", line 1, in
  File "resource://jsbridge/modules/Bridge.jsm:147", line 16, in Bridge.prototype.execFunctio
  File "resource://jsbridge/modules/Bridge.jsm:140", line 10, in Bridge.prototype._execFunctio
  File "resource://mozmill/modules/frame.js:778", line 3, in runTestFil
  File "resource://mozmill/modules/frame.js:693", line 3, in Runner.prototype.runTestFil
  File "resource://mozmill/modules/frame.js:709", line 9, in Runner.prototype.runTestModul
  File "resource://mozmill/modules/frame.js:755", line 5, in Runner.prototype.execFunctio
  File "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tests/testAddonsBlocklist.js:41", line 3, in testBlocklistAP
  File "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tests/testAddonsBlocklist.js:45", line 5, in testBlocklistAPI/
  File "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/modal-dialog.js:202", line 1, in modalDialog_waitForDialo
  File "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/assertions.js:555", line 10, in Assert_waitFo
  File "resource://mozmill/stdlib/utils.js:269", line 3, in waitFo
  File "resource://mozmill/modules/assertions.js:595", line 7, in Assert_waitFo
  File "resource://gre/modules/Promise-backend.js:688", line 37, in this.PromiseWalker.scheduleWalkerLoop/
  File "resource://gre/modules/Promise-backend.js:746", line 7, in this.PromiseWalker.walkerLoo
  File "resource://gre/modules/Promise-backend.js:867", line 23, in Handler.prototype.proces
  File "resource://gre/modules/addons/XPIProviderUtils.js:1069", line 9, in this.XPIDatabase.getAddon/
  File "resource://gre/modules/addons/XPIProviderUtils.js:126", line 5, in getRepositoryAddo
  File "resource://gre/modules/addons/XPIProviderUtils.js:145", line 17, in makeSafe/
  File "resource://gre/modules/addons/XPIProvider.jsm:5643", line 11, in downloadCompleted_getVisibleAddonForI
  File "resource://gre/modules/AddonManager.jsm:2522", line 1, in AMP_callInstallListener
  File "resource://gre/modules/AddonManager.jsm:1545", line 15, in AMI_callInstallListener
  File "resource://gre/components/amWebInstallListener.js:239", line 5, in Installer_onDownloadEnde
  File "resource://gre/components/amWebInstallListener.js:184", line 7, in Installer_checkAllDownloade
  File "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/modal-dialog.js:105", line 9, in mdObserver_observ
  File "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/addons.js:1745", line 1, in handleInstallAddonDialo
  File "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/lib/assertions.js:555", line 10, in Assert_waitFo
  File "resource://mozmill/stdlib/utils.js:269", line 3, in waitFo
  File "resource://mozmill/modules/assertions.js:595", line 7, in Assert_waitFo
  File "resource://gre/modules/Promise-backend.js:688", line 37, in this.PromiseWalker.scheduleWalkerLoop/
  File "resource://gre/modules/Promise-backend.js:746", line 7, in this.PromiseWalker.walkerLoo
  File "resource://gre/modules/Promise-backend.js:867", line 23, in Handler.prototype.proces
  File "resource://gre/modules/Task.jsm:330", line 41, in TaskImpl_ru
  File "resource://gre/modules/addons/XPIProvider.jsm:5846", line 13, in AI_startInstall/
  File "resource://gre/modules/addons/XPIProvider.jsm:4440", line 9, in XPI_callBootstrapMetho
  File "file:///var/folders/md/qy7y7r3d2zn7wfmcpvc2mtk80000gn/T/tmpRwY5w3.mozrunner/extensions/restartless-addon@quality.mozilla.org/bootstrap.js:138", line 5, in startu
  File "file:///var/folders/md/qy7y7r3d2zn7wfmcpvc2mtk80000gn/T/tmpRwY5w3.mozrunner/extensions/restartless-addon@quality.mozilla.org/bootstrap.js:101", line 5, in setupHarnes
  File "file:///var/folders/md/qy7y7r3d2zn7wfmcpvc2mtk80000gn/T/tmpRwY5w3.mozrunner/extensions/restartless-addon@quality.mozilla.org/components/harness.js:300", line 21, in Harness_loa
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:589", line 16, in requir
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:417", line 19, in asyncRequir
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:255", line 19, in syncRequir
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:337", line 17, in loadMaybeMagicModul
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:373", line 10, in loadFromModuleDat
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:170", line 1, in evaluat
  File "resource://gre/components/multiprocessShims.js:130", line 52, in AddonInterpositionService.prototype.interpose/desc.valu
  File "resource://gre/modules/RemoteAddonsParent.jsm:688", line 60, in ComponentsUtilsInterposition.methods.evalInSandbo
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-restartless-addon-lib/main.js:1", line 23, in
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:417", line 19, in asyncRequir
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:247", line 1, in syncRequir
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:337", line 17, in loadMaybeMagicModul
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:373", line 10, in loadFromModuleDat
  File "resource://restartless-addon-at-quality-dot-mozilla-dot-org-api-utils-lib/securable-module.js:170", line 1, in evaluat
  File "resource://gre/components/multiprocessShims.js:130", line 52, in AddonInterpositionService.prototype.interpose/desc.valu
  File "resource://gre/modules/RemoteAddonsParent.jsm:688", line 60, in ComponentsUtilsInterposition.methods.evalInSandbo
TypeError: redeclaration of formal parameter opt
[1;32mTEST-PASS[0m | testAddonsBlocklist.js | testBlocklistAPI
[1;34mTEST-START[0m | testAddonsBlocklist.js | teardownModule
TEST-END | testAddonsBlocklist.js | finished in 4915ms
[1;34mTEST-START[0m | testAddonsDiscoveryPane.js | setupModule
[1;34mTEST-START[0m | testAddonsDiscoveryPane.js | testAddonsAPI
[1;31mERROR[0m | Test Failure | {
  "fail": {
    "message": "There have to be 6 different sections - '7' should equal '6'", 
    "fileName": "file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tests/testAddonsDiscoveryPane.js", 
    "name": "testAddonsAPI", 
    "lineNumber": 23
  }
}
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "section is undefined", 
    "lineNumber": 27, 
    "name": "TypeError", 
    "fileName": "resource://mozmill/modules/frame.js -> file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tests/testAddonsDiscoveryPane.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testAddonsDiscoveryPane.js | testAddonsAPI
TEST-END | testAddonsDiscoveryPane.js | finished in 5114ms
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Module \"../../../../addons\" not found", 
    "lineNumber": 209, 
    "name": "Error", 
    "fileName": "resource://mozmill/stdlib/securable-module.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testAddonsManager.js | <TOP_LEVEL>
[1;34mTEST-START[0m | testAddonsManager.js | setupModule
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "addons is undefined", 
    "lineNumber": 22, 
    "name": "TypeError", 
    "fileName": "resource://mozmill/modules/frame.js -> file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tests/testAddonsManager.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testAddonsManager.js | setupModule
[1;34mTEST-START[0m | testAddonsManager.js | testOpenAMO
[1;33mTEST-SKIPPED[0m | testOpenAMO | setupModule failed
[1;34mTEST-START[0m | testAddonsManager.js | testAddonsAPI
[1;33mTEST-SKIPPED[0m | testAddonsAPI | setupModule failed
TEST-END | testAddonsManager.js | finished in 5ms
[1;34mTEST-START[0m | testBaseInContentPage.js | setupModule
[1;34mTEST-START[0m | testBaseInContentPage.js | testBaseInContentPage
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Tab has been opened", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testBaseInContentPage.js | testBaseInContentPage
[1;34mTEST-START[0m | testBaseInContentPage.js | teardownModule
TEST-END | testBaseInContentPage.js | finished in 6583ms
[1;34mTEST-START[0m | testBrowserWindow.js | setupModule
[1;34mTEST-START[0m | testBrowserWindow.js | testBrowserWindow
[1;32mTEST-PASS[0m | testBrowserWindow.js | testBrowserWindow
[1;34mTEST-START[0m | testBrowserWindow.js | teardownModule
TEST-END | testBrowserWindow.js | finished in 4047ms
[1;34mTEST-START[0m | testDownloadFileOfUnknownType.js | setupModule
[1;34mTEST-START[0m | testDownloadFileOfUnknownType.js | testDownloadFileOfUnknownType
[1;32mTEST-PASS[0m | testDownloadFileOfUnknownType.js | testDownloadFileOfUnknownType
[1;34mTEST-START[0m | testDownloadFileOfUnknownType.js | teardownModule
TEST-END | testDownloadFileOfUnknownType.js | finished in 1558ms
[1;34mTEST-START[0m | testDownloadsPanel.js | setupModule
[1;34mTEST-START[0m | testDownloadsPanel.js | testDownloadPanel
[1;32mTEST-PASS[0m | testDownloadsPanel.js | testDownloadPanel
[1;34mTEST-START[0m | testDownloadsPanel.js | teardownModule
TEST-END | testDownloadsPanel.js | finished in 1751ms
[1;34mTEST-START[0m | testIdentity.js | setupModule
[1;34mTEST-START[0m | testIdentity.js | testIdentity
[1;32mTEST-PASS[0m | testIdentity.js | testIdentity
[1;34mTEST-START[0m | testIdentity.js | teardownModule
TEST-END | testIdentity.js | finished in 1377ms
[1;34mTEST-START[0m | testMenuPanel.js | setupModule
[1;34mTEST-START[0m | testMenuPanel.js | testMenuPanel
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Preferences panes have been initialized", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testMenuPanel.js | testMenuPanel
[1;34mTEST-START[0m | testMenuPanel.js | teardownModule
TEST-END | testMenuPanel.js | finished in 6639ms
[1;34mTEST-START[0m | testPageInfoWindow.js | setupModule
[1;34mTEST-START[0m | testPageInfoWindow.js | testPageInfoWindow
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Category 'media' has been selected", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testPageInfoWindow.js | testPageInfoWindow
[1;34mTEST-START[0m | testPageInfoWindow.js | teardownModule
TEST-END | testPageInfoWindow.js | finished in 7391ms
[1;34mTEST-START[0m | testPrefs.js | setupModule
[1;34mTEST-START[0m | testPrefs.js | testPrefHelperClass
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Window has been found.", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testPrefs.js | testPrefHelperClass
TEST-END | testPrefs.js | finished in 5042ms
[1;34mTEST-START[0m | testSearch.js | setupModule
[1;34mTEST-START[0m | testSearch.js | testSearchAPI
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "Search engines drop down open state has been changed. Expected: 'true'", 
    "lineNumber": 27, 
    "name": "TimeoutError", 
    "fileName": "resource://mozmill/modules/errors.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testSearch.js | testSearchAPI
[1;34mTEST-START[0m | testSearch.js | teardownModule
TEST-END | testSearch.js | finished in 5145ms
[1;34mTEST-START[0m | testSessionStore.js | setupModule
[1;34mTEST-START[0m | testSessionStore.js | testAboutSessionRestoreErrorPage
[1;32mTEST-PASS[0m | testSessionStore.js | testAboutSessionRestoreErrorPage
TEST-END | testSessionStore.js | finished in 1138ms
[1;34mTEST-START[0m | testTabView.js | setupModule
[1;34mTEST-START[0m | testTabView.js | testTabViewClass
[1;31mERROR[0m | Test Failure | {
  "exception": {
    "message": "groupByTitle is undefined", 
    "lineNumber": 57, 
    "name": "TypeError", 
    "fileName": "resource://mozmill/modules/frame.js -> file:///Users/srm/Documents/sandeep/cst/mozilla/qa-mozmill-tests/firefox/lib/tests/testTabView.js"
  }
}
[1;31mTEST-UNEXPECTED-FAIL[0m | testTabView.js | testTabViewClass
TEST-END | testTabView.js | finished in 1188ms
[1;34mTEST-START[0m | testTabs.js | setupModule
[1;34mTEST-START[0m | testTabs.js | testCloseAllTabs
[1;32mTEST-PASS[0m | testTabs.js | testCloseAllTabs
[1;34mTEST-START[0m | testTabs.js | testFindBarAPI
[1;32mTEST-PASS[0m | testTabs.js | testFindBarAPI
[1;34mTEST-START[0m | testTabs.js | testTabs
[1;32mTEST-PASS[0m | testTabs.js | testTabs
[1;34mTEST-START[0m | testTabs.js | testTabSelect
[1;32mTEST-PASS[0m | testTabs.js | testTabSelect
[1;34mTEST-START[0m | testTabs.js | teardownModule
TEST-END | testTabs.js | finished in 8203ms
[1;34mTEST-START[0m | testToolbar.js | setupModule
[1;34mTEST-START[0m | testToolbar.js | testLocationBarAPI
[1;32mTEST-PASS[0m | testToolbar.js | testLocationBarAPI
TEST-END | testToolbar.js | finished in 1311ms
[1;34mTEST-START[0m | testUnknownContentTypeDialog.js | setupModule
[1;34mTEST-START[0m | testUnknownContentTypeDialog.js | testUnknownContentTypeDialog
[1;32mTEST-PASS[0m | testUnknownContentTypeDialog.js | testUnknownContentTypeDialog
[1;34mTEST-START[0m | testUnknownContentTypeDialog.js | teardownModule
TEST-END | testUnknownContentTypeDialog.js | finished in 2562ms
RESULTS | Passed: 12
RESULTS | Failed: 11
RESULTS | Skipped: 2

    
    

    
  
Please see the two posts containing the test output relating to lib and firefox/lib (run against Firefox Nightly).  I am unable to post the output for some others because of comment limits, but if you want I can put them all on my Gist pages - I have a separate text file for each set of tests - functional, endurance, remote, update, addons etc.

    
    

    
  
Please try to avoid those long comments. For those cases you can simply add attachments with all the log data contained. Anyhow I think the library tests are a bit busted so lets only focus your time on executing functional, and remote tests. If those pass we should be fine.

    
    

    
  
Comment on attachment 8556446 [details] [diff] [review]
bugfix-1088041-d.patch

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

I went deeply through this patch because I think it is time to do an actual review and not feedback. You may also want to request r? for upcoming patches. So the patch itself looks good now, all necessary changes seem to be included. So your rebasing seems to work fine now. Otherwise I have added a couple of comments, so please address them and make sure to run the requested tests before you attach a new version of the patch. Thanks!

::: firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
@@ +113,4 @@
>  
>    for (var i = 0; i < pluginNames.length; i++) {
>      if (pluginNames[i].textContent === aPluginName) {
> +      var properties = ["chrome://browser/locale/browser.properties"];

You are using a different properties file here, which will break the test.

::: firefox/tests/functional/testSecurity/testGreyLarry.js
@@ +85,5 @@
>                 "The owner label should equal the security owner");
>  
>    var webIDVerifierLabel = new elementslib.ID(aController.window.document,
>                                                "security-identity-verifier-value");
> +  var securityIdentifier = utils.getProperty(dtds, "notset");

This has to use properties.

::: firefox/tests/remote/testSecurity/testDVCertificate.js
@@ +108,4 @@
>    assert.ok(securityTab.getNode().selected, "The Security tab is selected by default");
>  
>    // Check the Web Site label against the Cert CName
> +  var webIDDomainLabel = new elementslib.ID(aController.window.document, "security-identity-domain-value");

Please revert this unrelated change.

@@ +112,5 @@
>    var certName = (cert.commonName.replace(/\./g, "\\\.")).replace(/\*/g, ".*");
>    var certNameRegExp = new RegExp("^" + certName + "$");
>  
>    expect.match(webIDDomainLabel.getNode().value, certNameRegExp,
> +"Expected web site label found");

Same here.

@@ +117,3 @@
>  
>    // Check the Owner label for "This web site does not supply ownership information."
> +  var webIDOwnerLabel = new elementslib.ID(aController.window.document, "security-identity-owner-value");

And here.

@@ +118,5 @@
>    // Check the Owner label for "This web site does not supply ownership information."
> +  var webIDOwnerLabel = new elementslib.ID(aController.window.document, "security-identity-owner-value");
> +  var properties = ["chrome://browser/locale/pageInfo.properties"];
> +  var securityOwner = utils.getProperty(properties, "securityNoOwner");
> +  expect.equal(webIDOwnerLabel.getNode().value, securityOwner, "Expected owner label found");

And here.

@@ +123,4 @@
>  
>    // Check the Verifier label against the Cert Issuer
> +  var webIDVerifierLabel = new elementslib.ID(aController.window.document, "security-identity-verifier-value");
> +  expect.equal(webIDVerifierLabel.getNode().value, cert.issuerOrganization, "Expected verifier label found");

And here.

::: firefox/tests/remote/testSecurity/testGreenLarry.js
@@ -116,4 @@
>    l10nVerifierLabel = l10nVerifierLabel.replace("%S", cert.issuerOrganization);
>    var verifier = identityPopup.getElement({type: "verifier"});
> -  expect.equal(verifier.getNode().textContent, l10nVerifierLabel,
> -               "The 'Verified by: %S' string is set");

Please revert this unrelated change here.

@@ +118,2 @@
>  
> +  var l10nEncryptionLabel = utils.getProperty(dtds, "identity.encrypted2");

This has to use the properties variable.

::: lib/tests/testUtils.js
@@ +60,4 @@
>    }, undefined, "Non-existent entity has not been found");
>  
>    // Test getEntity with a single string
> +  var entity = utils.getEntity([TEST_DATA.dtds.urls[0]], TEST_DATA.dtds.ids[0]);

We do not support strings anymore as parameters, so you can remove those two tests completely.

::: lib/utils.js
@@ -321,4 @@
>  
>    // Add xhtml11.dtd to prevent missing entity errors with XHTML files
>    urls.push("resource:///res/dtd/xhtml11.dtd");
> -

Please leave this blank line.

::: metrofirefox/tests/functional/testPopups/testAlwaysAllow.js
@@ -45,5 @@
>      controller.open(TEST_DATA + POPUPS_COUNT);
>      controller.waitForPageLoad();
>    });
> -
> -  var buttonLabel = utils.getProperty("chrome://browser/locale/browser.properties",

Please leave the above empty line to separate the code blocks. This also applies to other files, so please check them all for that.

        Attachment #8556446 -
        Flags: feedback?(hskupin) → review-

    
    

    
  
Thanks for the review.  First note that I have put the output of all the tests I did on 29/01 in a Gist text file here at https://gist.github.com/sr-murthy/581fa3babf9db516245e.

I have some questions about your review comments and suggestions.

1. 
>::: firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
>@@ +113,4 @@
>>  
>>    for (var i = 0; i < pluginNames.length; i++) {
>>      if (pluginNames[i].textContent === aPluginName) {
>> +      var properties = ["chrome://browser/locale/browser.properties"];
>
>You are using a different properties file here, which will break the test.

OK, but which properties file am I supposed to be using?

2. 
>::: firefox/tests/functional/testSecurity/testGreyLarry.js
>@@ +85,5 @@
>>                 "The owner label should equal the security owner");
>>  
>>    var webIDVerifierLabel = new elementslib.ID(aController.window.document,
>>                                                "security-identity-verifier-value");
>> +  var securityIdentifier = utils.getProperty(dtds, "notset");
>
>This has to use properties.

You mean the variable 'properties = ["chrome://browser/locale/pageInfo.properties"];' defined earlier on line 82?

3.
>::: firefox/tests/remote/testSecurity/testDVCertificate.js
>@@ +108,4 @@
>>    assert.ok(securityTab.getNode().selected, "The Security tab is selected by default");
>>  
>>    // Check the Web Site label against the Cert CName
>> +  var webIDDomainLabel = new elementslib.ID(aController.window.document, "security-identity-domain-value");
>
>Please revert this unrelated change.

I am not clear here, you want me to remove the line

  var webIDDomainLabel = new elementslib.ID(aController.window.document, "security-identity-domain-value");

from firefox/tests/remote/testSecurity/testDVCertificate.js?  I did not make this change.

4. 
>::: firefox/tests/remote/testSecurity/testGreenLarry.js
>@@ -116,4 @@
>>    l10nVerifierLabel = l10nVerifierLabel.replace("%S", cert.issuerOrganization);
>>    var verifier = identityPopup.getElement({type: "verifier"});
>> -  expect.equal(verifier.getNode().textContent, l10nVerifierLabel,
>> -               "The 'Verified by: %S' string is set");
>
>Please revert this unrelated change here.

I did not make this change.  You want me to add back the deleted lines?

>>@@ +118,2 @@
>>  
>> +  var l10nEncryptionLabel = utils.getProperty(dtds, "identity.encrypted2");
>
>This has to use the properties variable.

You mean the variable 'properties = ["chrome://browser/locale/browser.properties"];' defined earlier on line 106?

    
    

    
  
(In reply to Sandeep Murthy from comment #76)
> Thanks for the review.  First note that I have put the output of all the
> tests I did on 29/01 in a Gist text file here at
> https://gist.github.com/sr-murthy/581fa3babf9db516245e.

First I would suggest you compare the test results with and without your changes. That way you can see if new failures have been introduced. Also I would suggest you go through those failures and check if a newly introduced one is related, e.g. for the functional tests I can see a failure like "dtds is not defined". This is clearly a regression from your patch. Otherwise there is no need to run the add-on tests. If all the others pass, we are fine.

> >::: firefox/tests/functional/testAddons/testPluginDisableAffectsAboutPlugins.js
> >@@ +113,4 @@
> >>  
> >>    for (var i = 0; i < pluginNames.length; i++) {
> >>      if (pluginNames[i].textContent === aPluginName) {
> >> +      var properties = ["chrome://browser/locale/browser.properties"];
> >
> >You are using a different properties file here, which will break the test.
> 
> OK, but which properties file am I supposed to be using?

Please check what was used before and what you make use of now. You will see the difference.

> >::: firefox/tests/functional/testSecurity/testGreyLarry.js
> >@@ +85,5 @@
> >>                 "The owner label should equal the security owner");
> >>  
> >>    var webIDVerifierLabel = new elementslib.ID(aController.window.document,
> >>                                                "security-identity-verifier-value");
> >> +  var securityIdentifier = utils.getProperty(dtds, "notset");
> >
> >This has to use properties.
> 
> You mean the variable 'properties =
> ["chrome://browser/locale/pageInfo.properties"];' defined earlier on line 82?

Yes. You call .getProperty() here, so the URLs you have to pass-in have to contain properties but not DTD entities. This is also the failure as seen in the test results, as what I already have mentioned above.

> >::: firefox/tests/remote/testSecurity/testDVCertificate.js
> >@@ +108,4 @@
> >>    assert.ok(securityTab.getNode().selected, "The Security tab is selected by default");
> >>  
> >>    // Check the Web Site label against the Cert CName
> >> +  var webIDDomainLabel = new elementslib.ID(aController.window.document, "security-identity-domain-value");
> >
> >Please revert this unrelated change.
> 
> I am not clear here, you want me to remove the line

Revert means that you should restore the content as it was before your change. It should not appear in this diff.

>   var webIDDomainLabel = new elementslib.ID(aController.window.document,
> "security-identity-domain-value");
> 
> from firefox/tests/remote/testSecurity/testDVCertificate.js?  I did not make
> this change.

But that's what your diff shows. Not sure if someone else changed those lines so I would suggest that you pull master and rebase your branch. If the changes are still persistent they came in from you, maybe accidental by your editor.

> >>@@ +118,2 @@
> >>  
> >> +  var l10nEncryptionLabel = utils.getProperty(dtds, "identity.encrypted2");
> >
> >This has to use the properties variable.
> 
> You mean the variable 'properties =
> ["chrome://browser/locale/browser.properties"];' defined earlier on line 106?

Same as above. And dtds is not defined, so that's why you have this test failure.

    
    

    
  
Hi, you are asking for detailed changes, I only have time for this on Saturday-Sunday (currently I'm a full time student on week days).

>>>First I would suggest you compare the test results with and without your changes. That way you can see >>>if new failures have been introduced. Also I would suggest you go through those failures and check if a >>>newly introduced one is related, e.g. for the functional tests I can see a failure like "dtds is not >>>defined". 

I assume you mean only the functional and remote tests.  To see the test results without my changes (the rolled up commits in the last patch) I would have to create a new branch from the existing working branch for this patch and do a git revert (I think).

> >>    // Check the Web Site label against the Cert CName
> >> +  var webIDDomainLabel = new elementslib.ID(aController.window.document, "security-identity-domain-value");
> >
> >Please revert this unrelated change.
> 
> I am not clear here, you want me to remove the line

>>>Revert means that you should restore the content as it was before your change. It should not appear in >>>this diff.


The line 'var webIDDomainLabel = new elementslib.ID(aController.window.document, "security-identity-domain-value");' is definitely not something I added, even by accident.  I have kept a text file of all files I looked at and changed, and a description of the changes I made in those files, it was all related to looking at the getProperty/getEntity method calls in utils.js.

Could the reason be that before making the patch I rebased the local master from the remote master and then rebased the working branch against the local master?  But here I was following the instructions on this page https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests in the section 'Rebase against latest changes to mozmill-tests'?

    
    

    
  
Sorry I meant that before making the patch one of the steps I followed is that I updated the local master from the remote master (GitHub) and then rebased the working branch against the updated local master, following the instructions on 'Rebase against latest changes to mozmill-tests'.

    
    

    
  
(In reply to Sandeep Murthy from comment #78)
> Hi, you are asking for detailed changes, I only have time for this on
> Saturday-Sunday (currently I'm a full time student on week days).

No worries at all. Just find the time as needed and when you are free. There is no requirement to come up with an update immediately. It's just great that you work on it, and that you can learn a lot about versioning systems at the moment.

> I assume you mean only the functional and remote tests.  To see the test
> results without my changes (the rolled up commits in the last patch) I would
> have to create a new branch from the existing working branch for this patch
> and do a git revert (I think).

No, you don't have. Just switch back to the master branch which will not contain any of your changes. Also as you said in your last comment, pull from upstream/master first to ensure that you have the latest code.

> >>>Revert means that you should restore the content as it was before your change. It should not appear in >>>this diff.
> 
> The line 'var webIDDomainLabel = new
> elementslib.ID(aController.window.document,
> "security-identity-domain-value");' is definitely not something I added,
> even by accident.  I have kept a text file of all files I looked at and
> changed, and a description of the changes I made in those files, it was all
> related to looking at the getProperty/getEntity method calls in utils.js.
> 
> Could the reason be that before making the patch I rebased the local master
> from the remote master and then rebased the working branch against the local
> master?  But here I was following the instructions on this page
> https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests in the
> section 'Rebase against latest changes to mozmill-tests'?

Well, if the same indentation is visible on your master something might be wrong. Maybe you compare it with upstream/master to ensure no broken changeset landed in your local master. As you can see at the following URL the upstream/master doesn't have the longish single line which extends the 80-char limit.

https://github.com/mozilla/qa-mozmill-tests/blob/master/firefox/tests/remote/testSecurity/testDVCertificate.js#L112

    
    

    
  
Apologies for the delay, I've been overseas for the last three weeks, and before that completing a Python course :) If these changes are as you still require then I will try to start on this on Monday, if that's OK.

    
    

    
  
Thanks Sandeep for the update. In the last couple of weeks we were heavily working on getting a couple of our Mozmill tests converted to Firefox UI Tests, which are written in Python. See the repository here: https://github.com/mozilla/firefox-ui-tests. 

So what does it mean for this bug... we want to stop doing any work for Mozmill tests but completely focus on the Python tests. That's why I will mark this bug as wontfix now. But given your newly learned expertise in Python you may be interested to work with us on the Firefox UI tests project. Please let me know about that, and we can figure out a good starting bug for you.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX

    
    

    
  
OK no probs, I am very interested in the Python work (I completed a Python course at MITx/edX and did quite well), and I am working on my own Python/Django project at https://github.com/sr-murthy/TPP.

So any Python work is welcome!
Resolution: WONTFIX → INVALID

    
    

    
  
Nice. So if you want to have a look check the following page: https://bugzilla.mozilla.org/buglist.cgi?resolution=---&query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&component=Firefox%20UI%20Tests&product=Mozilla%20QA&list_id=12106688

We don't have that many mentored bugs yet, but maybe you can find one you are interested in and which is not too hard. Maybe bug 1145112 as a starter.
Resolution: INVALID → WONTFIX

    
  
Product: Mozilla QA → Mozilla QA Graveyard

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: