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

VERIFIED FIXED

Status

Mozilla QA
Mozmill Tests
--
critical
VERIFIED FIXED
8 years ago
6 years ago

People

(Reporter: Ehsan, Assigned: whimboo)

Tracking

({intermittent-failure, regression})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozmill-test-failure])

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

8 years ago
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.
(Reporter)

Comment 1

8 years ago
I also filed bug 613799 to get those tests to run on the try server, so that this doesn't happen in the future.

Updated

8 years ago
OS: Mac OS X → Linux
(Assignee)

Comment 2

8 years ago
(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.
(Assignee)

Comment 4

8 years ago
(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?
(Assignee)

Updated

8 years ago
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
(Assignee)

Updated

8 years ago
Whiteboard: [mozmill-test-failure]
(Assignee)

Updated

8 years ago
No longer depends on: 611844
(Assignee)

Comment 5

8 years ago
Created attachment 492168 [details] [diff] [review]
Sync current set of Mozmill tests with m-c

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)
(Assignee)

Comment 6

8 years ago
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?
(Assignee)

Comment 7

8 years ago
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();
}
?
(Assignee)

Comment 9

8 years ago
Created attachment 492631 [details] [diff] [review]
Dummy restart test

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 10

8 years ago
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+
(Assignee)

Comment 11

8 years ago
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
(Assignee)

Comment 12

8 years ago
Created attachment 492769 [details] [diff] [review]
Remove testDefaultBookmarks test
Attachment #492769 - Flags: review?(jhammel)
(Assignee)

Updated

8 years ago
Whiteboard: [mozmill-test-failure] → [orange][mozmill-test-failure]

Comment 13

8 years ago
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+
(Assignee)

Comment 14

8 years ago
Created attachment 492783 [details] [diff] [review]
Patch for check-in

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+
(Assignee)

Comment 15

8 years ago
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
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 16

8 years ago
(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 → ---
(Assignee)

Comment 18

8 years ago
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

Comment 19

8 years ago
(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).
(Assignee)

Comment 20

8 years ago
Created attachment 493226 [details] [diff] [review]
Patch with readme.txt to keep restartTests folder

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)
(Assignee)

Comment 21

8 years ago
Created attachment 493227 [details] [diff] [review]
Patch with readme.txt to keep restartTests folder (v2)

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 22

8 years ago
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+
(Assignee)

Comment 23

8 years ago
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

Comment 24

8 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/96f033fe78c0
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED

Comment 25

8 years ago
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 → ---
(Assignee)

Comment 26

8 years ago
Created attachment 494220 [details] [diff] [review]
Dummy test to have 1 passing test
Attachment #494220 - Flags: review?
(Assignee)

Updated

8 years ago
Attachment #494220 - Flags: review? → review?(ctalbert)

Comment 27

8 years ago
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+
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 32

8 years ago
landed: http://hg.mozilla.org/mozilla-central/rev/a3559b626db1
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
(Assignee)

Comment 33

8 years ago
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
Keywords: intermittent-failure
Whiteboard: [orange][mozmill-test-failure] → [mozmill-test-failure]
You need to log in before you can comment on or make changes to this bug.