When we want to run multiple updates in a row the meta data we collect with software update tests should be put into an array. That way we have combined information for each step which has been run. Right now all meta data is directly assigned to the persisted object. Together with that change the update testrun class has to be updated. So both patches should land at the same time.

This patch updates our tests to allow the collecting of multiple updates in a row for the same test-run. On bug 534744 I will cover the work which is necessary to send this data up to brasstacks.

Comment on attachment 483559 [details] [diff] [review] Patch v1 That doesn't work. Some fields (update type and url) are missing in the report. Also I will have to revert the waitFor back to waitForEval.

Reverted to waitForEval to have a smooth transistion to Mozmill 1.5.1 and updated the meta data to now collect all the information. Please test the output on the command line. The persisted object will be dumped to the console.

Comment on attachment 483607 [details] [diff] [review] Patch v2 >+ //this._controller.waitFor(function() { >+ // return this.currentPage == step; >+ //}, "The wizard page '" + step + "' has been selected.", gTimeout, null, this); > } > } Please add a header comment explaining why this is commented out (like we usually do). Something like: // XXX: Bug <number> - Don't use waitFor() until Mozmill 1.5.1

Sorry, my fault. Makes sense. Using the same comment as Geo did for the other tests.

r+ for code review. Is there something I should be testing with this patch prior to check-in? (re: comment 3)

Simply run the tests with the testrun_update.py script and specify the local repository with the patch applied to your queue like: ./testrun_update.py --repository=/path/to/mozmill-tests/ --log=filename build Afterward check the log file and search for persisted. All the entries I have added in the patch should be present.

This seems to have worked. Here is an example of what I see in the log: DEBUG | mozmill.persist | {"updateType": "partial+fallback", "updateBuildId": "20101012104758", "isCompletePatch": false, "preLocale": "en-US", "preBuildId": "20100824144458", "updateVersion": "3.6.11", "preVersion": "3.6.9", "type": "minor", "preUserAgent": "Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.2.9) Gecko/20100824 Firefox/3.6.9", "channel": "release"}

This looks like if you haven't ran the test with the --repository option as given above. The result is the old version as what we have right now.

(In reply to comment #9) > This looks like if you haven't ran the test with the --repository option as > given above. The result is the old version as what we have right now. I did run it with the --repository=/path/to/mozmill-tests/ option. But it still behaved like this. Maybe there is a problem with your script. Please try it yourself on qa-set.

(In reply to comment #10) > (In reply to comment #9) > > This looks like if you haven't ran the test with the --repository option as > > given above. The result is the old version as what we have right now. > > I did run it with the --repository=/path/to/mozmill-tests/ option. But it > still behaved like this. Maybe there is a problem with your script. Please > try it yourself on qa-set. Here is the command I used: ./testrun_update.py --repository=/data/scripts/mozmill-tests/ --log=updateTestRun /data/testing/3.6/update_mac/firefox-3.6.9.en-US.dmg Note, I just used /data/scripts as my working directory. I pulled a copy of mozmill-tests there and applied your patch. I then ran the above command. You can try it yourself. I'd be surprised if you get a different result than I.

I had to update the tests and shared module depending on the CommonJS changes. Further I have enabled update logging which will help a lot when we have to investigate download problems. For testing do: 1. qimport the patch into the default branch 2. Execute the testrun_update.py script with the --repository option which points to the above local repository. Also add --report=file://report to log the results locally 3. C&P the result content to http://jsonviewer.stack.hu/

Here an example output.

Here is the report from the testrun I did, as requested. The following is the command I used: /data/testing/scripts/testrun_update.py --repository=/data/testing/ashughes/mozmill-tests --report=file:///data/testing/ashughes/testrun.log --channel=beta /data/testing/4.0/update_mac/firefox-4.0b6.en-US.dmg

feedback+ based on comment 14.

(In reply to comment #15) > feedback+ based on comment 14. My fault. You will need a slightly update to the testrun.py file to see the results in the report. :/ For now a --show-all should help which will dump the persisted object to the console. I will work on bug 534744 tomorrow.

Comment on attachment 491060 [details] [diff] [review] Patch v4 Mostly looks good, couple of quickie comments: >+ if (!persisted.updateIndex) { >+ persisted.updateIndex = 0; >+ persisted.updates = [ ]; >+ } else { >+ // Increase the current update index for any new update >+ persisted.updateIndex++; >+ } ... >+ persisted.updates[persisted.updateIndex] = { >+ build_pre : update.buildInfo, >+ patch : update.patchInfo, >+ fallback : false, >+ success : false >+ }; I'm a little concerned about your handling of the updateIndex check above. There are several teardownModules in which this block appears (that one is the line 46 one). -If- your flow leaves you in a position where you have initialized the first member of persisted.updates[0] as just above, and then you enter a teardownModule like above, the !persisted.updateIndex block will clear the array and you'll lose that first member. Even if you're mostly sure that flow doesn't exist, may be best to use "persisted.updateIndex === undefined" in case the flow changes in the future. That would affect each teardownModule in which you've used the above pattern. Also, naming like build_pre is non-standard for our code (vs. buildPre) but you've mentioned to me on IRC that that's because it's used by the dashboard, where it is the naming standard. I don't have a big opinion on which standard to follow in cases like that, though I'd tend towards "whoever establishes the structure" which in this case is our Javascript code. I'll let you make the final call, though. If you decide to change it, do a quick search for "_" through your patch and adjust naming accordingly. >-var testDirectUpdate_AppliedAndNoUpdatesFound = function() { >+function testDirectUpdate_AppliedAndNoUpdatesFound() { > // Open the software update dialog and wait until the check has been finished > update.openDialog(controller); > update.waitForCheckFinished(); > > // No updates should be offered now - filter out major updates > if (update.updatesFound) { > update.download(persisted.channel, false); > >- controller.assertJS("subject.newUpdateType != subject.lastUpdateType", >- {newUpdateType: update.updateType, lastUpdateType: persisted.type}); >+ controller.assert(function() { >+ return update.updateType != persisted[persisted.updateIndex].update.type; >+ }, "No more update of the same type offered."); Looks like persisted[persisted.updateIndex].update.type may be wrong, and it should be persisted.updates[persisted.updateIndex].something. >-var testFallbackUpdate_AppliedAndNoUpdatesFound = function() { >+function testFallbackUpdate_AppliedAndNoUpdatesFound() { > // Open the software update dialog and wait until the check has been finished > update.openDialog(controller); > update.waitForCheckFinished(); > > // No updates should be offered now - filter out major updates > if (update.updatesFound) { > update.download(persisted.channel, false); > >- controller.assertJS("subject.newUpdateType != subject.lastUpdateType", >- {newUpdateType: update.updateType, lastUpdateType: persisted.type}); >+ controller.assert(function() { >+ return update.updateType != persisted[persisted.updateIndex].update.type; >+ }, "No more update of the same type offered."); > } Same problem re: persisted[persisted.updateIndex]. Since I've seen it twice, please do a quick search for "persisted[" in your patch file in case there's some I've missed. With those changes, r=me. I'd appreciate knowing your take on the teardownModule() flow issue I raised though.

(In reply to comment #17) > -If- your flow leaves you in a position where you have initialized the first > member of persisted.updates[0] as just above, and then you enter a > teardownModule like above, the !persisted.updateIndex block will clear the > array and you'll lose that first member. I don't initialize updates[0] in setupModule. It only creates an empty array. So not sure if I can follow your comment completely. > Even if you're mostly sure that flow doesn't exist, may be best to use > "persisted.updateIndex === undefined" in case the flow changes in the future. > That would affect each teardownModule in which you've used the above pattern. I should probably use '"updateIndex" in persisted' to avoid the js strict failure. But lemme see if I can completely avoid to use the index and only work with the array. > Also, naming like build_pre is non-standard for our code (vs. buildPre) but > you've mentioned to me on IRC that that's because it's used by the dashboard, > where it is the naming standard. I would prefer the current notation, also because it makes it a bit more clear which data is getting sent as part of the report. Let me try to move all that into the shared module, so tests don't have to bother about it at all. > >+ return update.updateType != persisted[persisted.updateIndex].update.type; > >+ }, "No more update of the same type offered."); > > Looks like persisted[persisted.updateIndex].update.type may be wrong, and it > should be persisted.updates[persisted.updateIndex].something. Nice catch. That's definitely wrong.

Move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.

(In reply to comment #18) > (In reply to comment #17) > > -If- your flow leaves you in a position where you have initialized the first > > member of persisted.updates[0] as just above, and then you enter a > > teardownModule like above, the !persisted.updateIndex block will clear the > > array and you'll lose that first member. > > I don't initialize updates[0] in setupModule. It only creates an empty array. > So not sure if I can follow your comment completely. You have code in teardownModule() (not setup) as I listed above: if !updateIndex, empty the array. It looks like later in the flow you then have code that says persisted.updates[updateIndex] = {some object}. At that point, updateIndex is still zero. If the next thing that happens is -another- teardownModule check, it'll re-empty the array. If this can happen, it's a bug. I can't tell from here because I don't know the whole flow. So I'm leaving it to you as this: If teardownModule() with that comparison can get entered with persisted.updateIndex == 0, you have a bug. > failure. But lemme see if I can completely avoid to use the index and only work > with the array. That might be the best option. At this point, I see nothing that we have to change, aside from updateIndex comparison if that bug is possible, so still r=me.

(In reply to comment #20) > At that point, updateIndex is still zero. If the next thing that happens is > -another- teardownModule check, it'll re-empty the array. Nope, because it will only reset the array if the property isn't defined. But I have changed it now in my updated patch to even stop the strict js error to pop-up.

The attached is the log from a testrun with Patch v5 applied. Here is the command I ran: /data/scripts/testrun_update.py --repository=/data/testing/ashughes/mozmill-tests --report=file:///data/testing/ashughes/testrun.log --channel=beta /data/testing/4.0/update_mac/firefox-4.0b6.en-US.dmg Here are the wikified results: Results: ======== * => , None, , unknown type, beta, 2010-11-29, '''FAIL''' ** ID: ** ID: ** Passed 5 :: Failed 0 :: Skipped 0 * => , None, , unknown type, beta, 2010-11-29, '''FAIL''' ** ID: ** ID: ** Passed 8 :: Failed 0 :: Skipped 0 I think it's interesting to note that the tests pass but report as failed with no build information.

Anthony, are you sure that you have patched the automation scripts? An output like that should not happen and needs to get a feedback-. Can you please double check?

Well, when checking the attached report, I don't see any meta data collected by the update tests. That means the patch for the automation script had been applied, but have you run the tests with the --repository option?

(In reply to comment #23) > Anthony, are you sure that you have patched the automation scripts? An output > like that should not happen and needs to get a feedback-. Can you please double > check? Yes, I ran this 3 times and each time I get the same wikified output. (In reply to comment #24) > Well, when checking the attached report, I don't see any meta data collected by > the update tests. That means the patch for the automation script had been > applied, but have you run the tests with the --repository option? See the command I pasted in comment 22, the --repository option is clearly visible.

(In reply to comment #21) > Created attachment 493469 [details] [diff] [review] > Patch v5 > > (In reply to comment #20) > > At that point, updateIndex is still zero. If the next thing that happens is > > -another- teardownModule check, it'll re-empty the array. > > Nope, because it will only reset the array if the property isn't defined. But I > have changed it now in my updated patch to even stop the strict js error to > pop-up. Keep in mind !myProp evaluates to true if myProp is undefined/null OR if myProp == 0. That was exactly my concern. I like the "in" you've done instead. I'm convinced that will be more solid. Interdiff looks like, r+ on the patch.

*looks fine, that is

Ok, looks like I forgot to apply the other metadata patch as well. Now that is applied I get the proper wiki output and "updates" object. feedback+

Simple backport to 1.9.2. Anthony, can you please perform the same steps of testing as for the default case?

Backport and small updates to the old module version and tests.

Comment on attachment 493829 [details] [diff] [review] Backport v1 (1.9.2) Report and wiki results same as with default. feedback+

Comment on attachment 493850 [details] [diff] [review] Backport v1 (1.9.1) Report and wiki results same as on default. feedback+

Comment on attachment 493829 [details] [diff] [review] Backport v1 (1.9.2) Looks fine. r+

Comment on attachment 493850 [details] [diff] [review] Backport v1 (1.9.1) Also looks fine. r+

Patches have been landed on all branches: http://hg.mozilla.org/qa/mozmill-tests/rev/9fa2aa8bcb44 (default) http://hg.mozilla.org/qa/mozmill-tests/rev/21401ef2bbd7 (1.9.2) http://hg.mozilla.org/qa/mozmill-tests/rev/3e1dbbb3ad76 (1.9.1)