Closed Bug 613802 Opened 14 years ago Closed 14 years ago

Landing of Firefox button on Linux has broken mozmill/tests/firefox/restartTests/testDefaultBookmarks/test1.js | testVerifyDefaultBookmarks

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
critical

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: ehsan.akhgari, Assigned: whimboo)

References

Details

(Keywords: intermittent-failure, regression, Whiteboard: [mozmill-test-failure])

Attachments

(2 files, 5 obsolete files)

Bug 585370 broke the Mozmill suite on trunk:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1290301692.1290302125.20164.gz

I've hidden the Mozmill boxes on mozilla-central for now.  They should be unhidden when this problem is fixed.
I also filed bug 613799 to get those tests to run on the try server, so that this doesn't happen in the future.
OS: Mac OS X → Linux
(In reply to comment #0)
> Bug 585370 broke the Mozmill suite on trunk:

It only breaks one test. But what's the standard procedure when tests are failing? Shouldn't the patch be backed out then?

In any way, my patch on bug 611844 should fix it. We wanted to wait to push it m-c until the 2nd part is also ready, but looks like we should do this immediately.

Moving to Mozilla QA -> Mozmill Tests, because that's not a failure in the framework itself.
Assignee: nobody → hskupin
Component: Mozmill → Mozmill Tests
Depends on: 611844
Product: Testing → Mozilla QA
QA Contact: mozmill → mozmill-tests
Version: Trunk → unspecified
The standard procedure for tests that run on Try, that have a documented way for developers to run them, and that have a documented way to disable them, is to do one of three things: back out the patch, fix the test, or disable the test because it's making bad assumptions or was written in a way that's bound to fail.

The standard procedure for tests that do not run on Try, that nobody knows how to run themselves, and that nobody knows how to disable without just experimenting on mozilla-central, is to hide the test column.
(In reply to comment #3)
> The standard procedure for tests that run on Try, that have a documented way
> for developers to run them, and that have a documented way to disable them, is
> to do one of three things: back out the patch, fix the test, or disable the
> test because it's making bad assumptions or was written in a way that's bound
> to fail.

Thanks Phil! That information was helpful. Looks like we have to get bug 568642 in place as soon as possible, so everyone could run those tests easily. We should also add documentation right away. Jeff or Clint, can you please do that?
Blocks: 585370
Keywords: regression
Summary: Mozmill suite broken on mozilla-central (on Linux) after bug 585370 landed, in mozmill/tests/firefox/restartTests/testDefaultBookmarks/test1.js | testVerifyDefaultBookmarks → Landing of Firefox button on Linux has broken mozmill/tests/firefox/restartTests/testDefaultBookmarks/test1.js | testVerifyDefaultBookmarks
Whiteboard: [mozmill-test-failure]
No longer depends on: 611844
This patch updates the set of Mozmill tests in m-c we are running on buildbot:

* Get rid of the old module system in favor of CommonJS
* Fix the step to ope the bookmarks toolbar (bookmarks button is not present on all platforms) - also fixes the Firefox button on Linux problem
* Further enhancements to the default bookmark test to make it more robust
Attachment #492168 - Flags: review?(ctalbert)
Clint and Jeff, I think we could run into troubles when running the state of the tests with the version of Mozmill we currently use on buildbot. The reason is that it doesn't wait for page loads in background tabs.

Do we wanna release a bugfix release for bug 611347, so we simply suppress the error message?
Jeff, so to clear the current bad state on buildbot we could simply remove this test and place an empty test in the restart test-run. That way we wouldn't have to disable restart tests but also wouldn't get test failures. What do you think?
The test still works everywhere but Linux.  Presumably there's value in continuing to run it on the other platforms, or we wouldn't be running this in the first place.  Can the test do some sort of 

if (!platformIsLinux()) {
  doTest();
} else {
  doNothing();
}
?
Attached patch Dummy restart test (obsolete) — Splinter Review
As checked yesterday the defaultBookmark test accesses external content by clicking on the "Getting Started" bookmark. So it should not be part of the tests which get run on buildbot.

Because we have to run at least one test for restart tests we should simply stick a dummy test in the restartTests folder.
Attachment #492168 - Attachment is obsolete: true
Attachment #492631 - Flags: review?(jhammel)
Attachment #492168 - Flags: review?(ctalbert)
Comment on attachment 492631 [details] [diff] [review]
Dummy restart test

This is probably okay.  I hesitate doing such hackery, but can't think of a better solution for right now
Attachment #492631 - Flags: review?(jhammel) → review+
Comment on attachment 492631 [details] [diff] [review]
Dummy restart test

I had another talk with Jeff about the current situation and the behavior of the test-runs in case of no tests involved. I have executed the steps to get the mozmill tests to run in an environment, as buildbot is working in. I can't see any problem with the restart test-run if no tests are involved. The output I get is:

$ mozmill-restart -t tests/firefox/restartTests/
Skipping /obj/minefield/dist/test-package-stage/mozmill/tests/firefox/restartTests does not contain any known test file names
INFO Passed: 0
INFO Failed: 0
INFO Skipped: 0

$ echo $?
0

The exit code is zero, and nothing gets reported as failed. That means we can safely delete the test from the restartTests folder and don't have to add a dummy test.

I will come up with a new patch which is simply disabling this test.
Attachment #492631 - Attachment is obsolete: true
Attached patch Remove testDefaultBookmarks test (obsolete) — Splinter Review
Attachment #492769 - Flags: review?(jhammel)
Whiteboard: [mozmill-test-failure] → [orange][mozmill-test-failure]
Comment on attachment 492769 [details] [diff] [review]
Remove testDefaultBookmarks test

looks fine; i didn't get a chance to test but i'll trust henrik's judgement
Attachment #492769 - Flags: review?(jhammel) → review+
Attached patch Patch for check-in (obsolete) — Splinter Review
As mentioned by davidb on IRC I forgot to update the commit message. This patch fixes it and makes the patch ready for landing.
Attachment #492769 - Attachment is obsolete: true
Attachment #492783 - Flags: review+
Patch has been landed:
http://hg.mozilla.org/mozilla-central/rev/57e37b269291

I will watch the tree that we stay green. If that's the case we could re-enable mozmill results for Linux.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
(In reply to comment #13)
> Comment on attachment 492769 [details] [diff] [review]
> Remove testDefaultBookmarks test
> 
> looks fine; i didn't get a chance to test but i'll trust henrik's judgement

So, on closer inspection, this isn't going to work.

Fact A:  mercurial does not keep track of directories.  Since this patch removes testing/mozmill/tests/firefox/restartTests/testDefaultBookmarks/test1.js , and test1.js is the only file in testDefaultBookmarks and testDefaultBookmarks is the only file in restartTests, mozmill-restart will fail as invoked by buildbot: http://mxr.mozilla.org/build/source/buildbotcustom/process/factory.py#6922

(mozmill)> mozmill-restart -t tests/firefox/restartTests -b ../../bin/firefox --show-all
Traceback (most recent call last):
  File "/home/jhammel/mozilla/src/tmp/obj-browser/dist/test-package-stage/mozmill/bin/mozmill-restart", line 8, in <module>
    load_entry_point('mozmill==1.5.1rc2', 'console_scripts', 'mozmill-restart')()
  File "/home/jhammel/mozilla/src/tmp/obj-browser/dist/test-package-stage/mozmill/lib/python2.6/site-packages/mozmill/__init__.py", line 817, in restart_cli
    RestartCLI().run()
  File "/home/jhammel/mozilla/src/tmp/obj-browser/dist/test-package-stage/mozmill/lib/python2.6/site-packages/mozmill/__init__.py", line 727, in __init__
    raise Exception("Not a valid test file/directory")
Exception: Not a valid test file/directory

There might be a Fact B. as well, but that alone is enough to kill this.

I suppose there are a few lessons to learn here, which I will try to take to heart:

 - never r+ patches until you check them locally.  I noticed this when I did try the patch locally on a clean tree, but by then it was too late.

 - Try should resemble production.  The fact that it doesn't makes local checks succeptible to local environment issues.  :whimboo did not see this locally, and I have had similar issues with mercurial (e.g. not removing empty directories locally)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Jeff, given the brokenness from above, it would mean we have two options:

* Check-in a dummy test (as done in a former patch on this bug)

* Place a readme.txt file into the restartTests folder which tells that there are no tests atm. It can be removed once the first restart patch lands.
Status: REOPENED → ASSIGNED
(In reply to comment #18)
> Jeff, given the brokenness from above, it would mean we have two options:
> 
> * Check-in a dummy test (as done in a former patch on this bug)
> 
> * Place a readme.txt file into the restartTests folder which tells that there
> are no tests atm. It can be removed once the first restart patch lands.

I have no strong preference, as long as the fix works.  In either case its a temporary kludge to be avoided persisting (and something that would be fixed with manifests, bug 585106).
Running a dummy test will result in wrong test result numbers. Keeping a readme.txt with a description in the restartTests folder, makes it more obvious.
Attachment #492783 - Attachment is obsolete: true
Attachment #493226 - Flags: review?(jhammel)
Missed a qrefresh for the missing empty line.
Attachment #493226 - Attachment is obsolete: true
Attachment #493227 - Flags: review?(jhammel)
Attachment #493226 - Flags: review?(jhammel)
Comment on attachment 493227 [details] [diff] [review]
Patch with readme.txt to keep restartTests folder (v2)

tested and works
Attachment #493227 - Flags: review?(jhammel) → review+
Clint, can you please take care of the landing? I don't have permissions for it. Thanks.
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
Landed: http://hg.mozilla.org/mozilla-central/rev/96f033fe78c0
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
This approach did not work because buildbot counts failures and since the mozmill test reports that 0 items passed, buildbot marks this as a T-Fail.

The relevant buildbot code is here:
http://mxr.mozilla.org/build/source/buildbotcustom/steps/unittest.py#45

The log file from a green(!) build is here:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1291140444.1291140753.5812.gz&fulltext=1

REOPENING
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #494220 - Flags: review? → review?(ctalbert)
Comment on attachment 494220 [details] [diff] [review]
Dummy test to have 1 passing test

It's absolute craziness that we have to do this hack, but we will be improving this in the next version of mozmill, so I'm ok with doing this temporarily to get us back to green.  

Thanks for the patch.
Attachment #494220 - Flags: review?(ctalbert) → review+
landed: http://hg.mozilla.org/mozilla-central/rev/a3559b626db1
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
This time we did the right thing. All tests are passing again. Marking as verified fixed.

Thanks Clint for landing the patch!
Status: RESOLVED → VERIFIED
Keywords: checkin-needed
Whiteboard: [orange][mozmill-test-failure] → [mozmill-test-failure]
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: