767286 - Fix test, manifest, and directory names for Mutt tests to make the test collector work again

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

In bug 760418 I have reorganized the whole Mutt folder structure but now while working on the manifest and test code I have seen the following problems:

* Manifests should not be named as 'tests.ini' because mozmill will assume those are tests. 

* Test files should start with 'test'. We could change the behavior of the collector to only obey .js files, which is probably the best solution. Jeff, if you agree on that I will cover it by bug 762058.

* Directories have to start with 'test'. Personally I wouldn't limit those to names with the 'test' prefix but crawl all of them for possible tests.

Jeff, please give me feedback on it so I can start fixing that.

Comment 1

[Jeff Hammel]

I would actually be more inclined to disable the directory collection entirely.  IMHO, it is better to use manifests for this case and make '-t' work only with files.  If you want to run a large chunk of tests, manifests give you much more control.  ManifestDestiny already has some logic for generating manifests by crawling directories and I'd rather enhance this logic than putting it in mozmill.  If we can figure out what we want to do, I'm happy to implement that.

If we do go ahead with supporting crawling directories for tests, I would be more inclined to change our logic to do the following:

- taking tests according to if they start with 'test' is kinda bogus.  I would prefer taking files that end in '.js' or, if there is a strong reason to do so, taking files that start with 'test' and end in '.js' .  This will eliminate the first problem and it is probably better logic anyway

- We can continue crawl directories based on them starting with 'test'
** Alternative 1: We can crawl the directories regardless of name
** Alternative 2: We can crawl directories regardless of name but if a subdirectory has no test files, we don't descend further. Personally, I favor this option (though Alternative 1 is fine too)

To restate, I'm also open to moving as much of this logic as possible to ManifestDestiny.  We should figure out exactly what we want to do and then decide where to do it. The collection logic is mostly a straight port from the 1.5 series.  Since 2. is a huge refactor, I don't think we should feel bound to support backwards compatability here.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Disabling the old test collector we should do post 2.0 once everyone uses Manifests for their tests. For now it would be helpful to have it working again. So we have to do the following steps:

(In reply to Jeff Hammel [:jhammel] from comment #1)
> - taking tests according to if they start with 'test' is kinda bogus.  I
> would prefer taking files that end in '.js' or, if there is a strong reason
> to do so, taking files that start with 'test' and end in '.js' .  This will
> eliminate the first problem and it is probably better logic anyway

Sounds fine to me too. 

> - We can continue crawl directories based on them starting with 'test'
> ** Alternative 1: We can crawl the directories regardless of name
> ** Alternative 2: We can crawl directories regardless of name but if a
> subdirectory has no test files, we don't descend further. Personally, I
> favor this option (though Alternative 1 is fine too)

I wouldn't do that given that there might be sub folders with testcase data you don't want to crawl. Contained js files are not necessarily Mozmill tests. So we should keep the test prefix for sub folders.

I will put this up for mentoring given that it is a nice starter into the Mozmill. Information about how to get started you can find here:

https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Getting_Started

Comment 3

[Jonathan French (:jfrench)]

This sounds like a nice renaming project, similar to my re-naming of /py side  mutt tests in bug 870384 for consistency. I till take this bug and have a go at it. At a minimum, I could provide a patch from which someone could add additional collector work, if needed.

So within mutt/mutt/tests/, I gather we want in general:

- all mutt/mutt/tests/js/ tests to -> testName.js like our mozmill-tests
  eg. expect_assert.js -> testExpectAssert.js
- all mutt/mutt/tests/js/ test directories to -> testName/
  eg. assertions/ -> testAssertions/

- all js/ and py/ manifests to be renamed from test.ini -> manifest.ini

Anything else?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Great! Thanks Jonathan. Beside your proposed changes you should also update any manifest which is named 'tests.ini' to 'manifest.ini'. Otherwise the collector will assume that those are tests and Mozmill will fail.

Comment 5

[Jonathan French (:jfrench)]

I'm about halfway through the renaming, but had to detour into a bunch of bug verifications for the team. This bug is going well though. Realistically I hope to have a patch early next week after some thorough testing.

Comment 6

[Jonathan French (:jfrench)]

Attachment: rename mapping rev1.0

Here's a name mapping table attached of the upcoming change. it is basically a stripped git patch, re-ordered in a sensible and readable structure.

Let me know if you'd like any names or directories tweaked. I took initiative to change a couple of file names outside of the scope of the agreed renaming plan (but logical and consistent with existing files going forward). They are annotated in the attachment.

A couple of py side manifest files have also been included in this change, since we are committing to manifest.ini across the harness.

Interestingly, /driver and /metro are not present in the top level manifest in mutt/mutt/tests/js/. It appears they don't get run at all. If you'd like them added in, let me know, and in what order they should appear in that manifest.

Once we have the names and directories tuned, I will start local testing to make sure everything is running properly.

Comment 7

[Jonathan French (:jfrench)]

A minor update to the above renaming attachment. The proposed testHttpd/ renaming to test{1,2}.js were superseded by 2 new tests which have recently arrived in the httpd/ directory. So those 2 new tests will remain unchanged. They will also be merged in my upcoming patch.

Comment 8

[Jonathan French (:jfrench)]

Attachment: collector support renaming (master) rev1.0

Patch "collector support renaming (master) rev1.0" for Master branch. 177 files changed/added/removed.

This patch represents all the proposed rename mapping spec'd in the attachment in Comment 6. Please review that spec there if you need to see in ascii what has changed, as this git patch is pretty indecipherable. Or apply the patch locally and have a look at the directory results.

This patch is rebased to latest master. It also contains merges of several test.ini's which changed in the master repo during the work. Additionally it contains a merge of the 2 new tests in http/ - which is now renamed to testHttp/. The patch was tested to ensure all manifest's and test references, still talk to each other during the run.

mutt.py was altered as requested, and no longer uses all-tests.ini.

Tested numerous times with `mutt testall` and test results of clean master diffed vs. a branch run, to ensure the results were the same. They appear to be.

Adjustments also happened on the python/ side of the mutt tests, to ensure a consistent use of manifest.ini across the entire harness.

The repository for test files, formerly _files/, was renamed to data/, since that is what is being used in mozmill-tests/, and that appears to be the desired way of referring to external data going forward. I will do the same with the python side _files/, but would prefer to do that in a separate bug to keep the size of this patch down.

It would be worthwhile for someone with a case sensitive OS to do a mutt testall run with the patch, in case of upper or lower case filename discrepancies, which would be masked on my Windows platform. I took care to try to ensure they were correct.

Windows run results for reference:

Javascript Tests
Pass: 61
Fail: 0
Skip: 2

Total Tests
Total Pass: 80
Total Fail: 0
Total Skip: 2

These result counts above, are consistent with clean master.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch
latest Nightly 25.0a1 20130717030207 138720

Let me know if there is any naming you'd like adjusted.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 778223 [details] [diff] [review]
collector support renaming (master) rev1.0

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

Thanks Jonathan, this is looking great. I found it a lot easier to review this on github, so I pushed the changes to a branch on my fork. I haven't run the tests locally yet, but will once the commented items are addressed.

::: mutt/mutt/tests/js/manifest.ini
@@ +2,5 @@
> +type = javascript
> +
> +[include:testAssertions/manifest.ini]
> +[include:testController/manifest.ini]
> +[include:testElementslib/manifest.ini]

Could we rename this folder to testElementsLib?

::: mutt/mutt/tests/js/testElementslib/manifest.ini
@@ +1,5 @@
> +[DEFAULT]
> +type = javascript
> +
> +[testLookup.js]
> +[testMenulist.js]

Rename to testMenuList.js

@@ +2,5 @@
> +type = javascript
> +
> +[testLookup.js]
> +[testMenulist.js]
> +[testMozelement.js]

Rename to testMozElement.js

@@ +3,5 @@
> +
> +[testLookup.js]
> +[testMenulist.js]
> +[testMozelement.js]
> +[testMozelementWindow.js]

Rename to testMozElementWindow.js

::: mutt/mutt/tests/js/testFrame/testPythonCallback.js
@@ +6,5 @@
> +  controller = mozmill.getBrowserController();
> +}
> +
> +var test = function () {
> +  mozmill.firePythonCallback("test_python_callback.py", "check",

This isn't a test, so we shouldn't rename it as such.

::: mutt/mutt/tests/js/testFrame/testRestart/testStates/manifest.ini
@@ +1,4 @@
> +[DEFAULT]
> +type = javascript
> +
> +[testSetuptestPass.js]

Rename to testSetupTestPass.js

@@ +2,5 @@
> +type = javascript
> +
> +[testSetuptestPass.js]
> +
> +[testSetuptestSkip.js]

Rename to testSetupTestSkip.js

::: mutt/mutt/tests/js/testFrame/test_python_callback.py
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

As mentioned, this isn't a test and shouldn't be renamed.

::: mutt/mutt/tests/js/testUtils/manifest.ini
@@ +1,5 @@
> +[DEFAULT]
> +type = javascript
> +
> +[testWaitFor.js]
> +[testPageload.js]

Rename to testPageLoad.js

@@ +2,5 @@
> +type = javascript
> +
> +[testWaitFor.js]
> +[testPageload.js]
> +[testPageloadBfcache.js]

Rename to testPageLoadBfCache.js

Comment 10

[Jonathan French (:jfrench)]

Awesome, thanks Dave. Changes and a revised patch coming up.

Comment 11

[Jonathan French (:jfrench)]

(In reply to Dave Hunt (:davehunt) from comment #9)
> > +[include:testElementslib/manifest.ini]
> 
> Could we rename this folder to testElementsLib?

Will make this change, but just for reference my initial choice was based on the usage in-code, which isn't mixed case.

eg.
let textbox = new elementslib.ID(controller.tabs.activeTab, "fname");

No probs though, am switching the directory name as directed.

Comment 12

[Jonathan French (:jfrench)]

Attachment: collector support renaming (master) rev1.1

Patch "collector support renaming (master) rev1.1" for Master. 177 files changed/added/removed.

All naming adjustments made as per comment 9. Retested and rebased again to latest master.

Windows run results for reference:

Javascript Tests
Pass: 61
Fail: 0
Skip: 2

Total Tests
Total Pass: 80
Total Fail: 0
Total Skip: 2

These result counts above, are consistent with latest clean master.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch rev1.1
latest Nightly 25.0a1 20130722030226 139327

Comment 13

[Jonathan French (:jfrench)]

The patch above has been made redundant by 7 commits in the past 72hrs since uploading. It will need rebasing.

Comment 14

[Jonathan French (:jfrench)]

Attachment: collector support renaming (master) rev1.2

Patch "collector support renaming (master) rev1.2" for Master branch. 169 files changed/added/removed.

Whew. Lots of files changed in the repo the last three days. Lots of resolves. Rebased again to latest master. Retested with `mutt testall`. There is one new failure introduced in master, and it shows up in both clean master, and in the patch test runs.

One unrelated correction has been included, removing testStateMachine.js from mutt/mutt/tests/js/frame/restart/tests.ini as it was unused.

Windows run results for reference:

Javascript Tests
Pass: 62
Fail: 1
Skip: 2

Total Tests
Total Pass: 85
Total Fail: 1
Total Skip: 2

These result counts above, are consistent with latest clean master.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch rev1.2
latest Nightly 25.0a1 20130724222711 139885

Comment 15

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #14)
> Created attachment 781056 [details] [diff] [review]
> collector support renaming (master) rev1.2
> 
> There is one new failure introduced in master, and it shows up in both clean
> master, and in the patch test runs.

And to clarify, that test that fails in both clean master and the patch in my runs is
js\elementslib\mozelement.js | test

[1;31mERROR[0m | Test Failure | {
  "exception": {
    "stack": [
      "BaseError@resource://mozmill/modules/errors.js:27", 
      "AssertionError@resource://mozmill/modules/errors.js:76", 
      "Assert_waitFor@resource://mozmill/modules/assertions.js:607", 
      "browserAdditions/controller.waitForPageLoad@resource://mozmill/driver/controller.js:991", 
      "test@resource://mozmill/modules/frame.js -> file:///c:/mozmill/master-bug-767286-rev1/mutt/mutt/tests/js/elementslib/mozelement.js:14", 
      "Runner.prototype.execFunction@resource://mozmill/modules/frame.js:756", 
      "Runner.prototype.runTestModule@resource://mozmill/modules/frame.js:710", 
      "Runner.prototype.runTestFile@resource://mozmill/modules/frame.js:694", 
      "runTestFile@resource://mozmill/modules/frame.js:779", 
      "Bridge.prototype._execFunction@resource://jsbridge/modules/Bridge.jsm:140", 
      "Bridge.prototype.execFunction@resource://jsbridge/modules/Bridge.jsm:147", 
      "@resource://jsbridge/modules/Server.jsm:32", 
      ""
    ], 
    "message": "controller.waitForPageLoad(): Timeout waiting for page loaded.", 
    "fileName": "resource://mozmill/modules/errors.js", 
    "name": "AssertionError", 
    "lineNumber": 27
  }
}

The test itself is unchanged in master for the past 2 months. It passed 3 days ago, so perhaps something in the intervening 7 commits to mozmill since Monday may be the culprit (theory). It is marked as an unexpected fail in the run.

That being said, this has no effect on the ability to land this patch submitted in Comment 14. It fails in the patch also, therefore is consistent.

Comment 16

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #15)

I've updated the relevant bug 871441 for the above failure.

So this patch review can proceed.

Comment 17

[Jonathan French (:jfrench)]

Attachment: collector support renaming (master) rev1.3

Patch "collector support renaming (master) rev1.3" for Master branch. 169 files changed/added/removed.

Whimboo backed out the commit for the bug discovered in bug 871441 and mentioned above in comment 15, so this required another rebase for this patch to land cleanly.

There are no other changes to the patch other than the rebase. Re-tested with mutt testall.

Let me know if there's any further adjustments needed.

Windows run results for reference:

Javascript Tests
Pass: 62
Fail: 0
Skip: 2

Total Tests
Total Pass: 85
Total Fail: 0
Total Skip: 2

These result counts above, are consistent with latest clean master.

Tested in:
Windows XP32 SP3
mozmill-env w/mozmill 2.0rc4 latest + patch rev1.2
latest Nightly 25.0a1 20130724222711 139885

Comment 18

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #17)
> Tested in:
> mozmill-env w/mozmill 2.0rc4 latest + patch rev1.2

Meant + patch rev1.3 here, of course.

Comment 19

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 781222 [details] [diff] [review]
collector support renaming (master) rev1.3

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

Nearly there, just a few comments, and sorry that I missed some of these last time. I ran the tests this time and can confirm the same results with/without the patch applied.

::: mutt/mutt/tests/js/manifest.ini
@@ +6,5 @@
> +[include:testElementsLib/manifest.ini]
> +[include:testFrame/manifest.ini]
> +[include:testL10n/manifest.ini]
> +[include:testUtils/manifest.ini]
> +[include:testLiveweb/manifest.ini]

Rename to testLiveWeb

::: mutt/mutt/tests/js/testFrame/manifest.ini
@@ +8,5 @@
> +[include:testRestart/manifest.ini]
> +[include:testSandbox/manifest.ini]
> +[include:testUserShutdown/manifest.ini]
> +
> +[testRequire/testRequire.js]

As we're using a subdirectory here, please create a manifest.ini file and reference that instead.

::: mutt/mutt/tests/js/testUtils/manifest.ini
@@ +1,5 @@
> +[DEFAULT]
> +type = javascript
> +
> +[testPageLoad.js]
> +[testPageLoadBfcache.js]

You missed to rename this to testPageLoadBfCache

Comment 20

[Jonathan French (:jfrench)]

(In reply to Dave Hunt (:davehunt) [away 07/29 - 08/02] from comment #19)
> Comment on attachment 781222 [details] [diff] [review]
> collector support renaming (master) rev1.3
> 
> Review of attachment 781222 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> > +
> > +[testPageLoad.js]
> > +[testPageLoadBfcache.js]
> 

Yup I got the "L" in the first rev1.1 but missed the "C" and it propagated forward. I'm out today but will fix this and the others over the weekend. And likely another rebase.

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

I just discussed with Henrik and seeing as there are just a couple of minor issues I'll fix them up and get a new patch for review today. We don't want you to have to rebase etc again, and this is so close. :)

Comment 22

[Jonathan French (:jfrench)]

Ok, will do, thanks Dave. Whichever is fine, the last rebase wasn't bad at all, I'm sure a weekend one would be fine for me also if you don't get to it today. It was just the second one which was long since the patch touches a lot of the repo.

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: collector support renaming (master) rev1.4

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 781717 [details] [diff] [review]
collector support renaming (master) rev1.4

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

Just curious. Why has this patch not been exported with format-patch? Please land it yourself Dave. I quickly spot-checked the remaining items from the last review and it looks good. So lets get this in.

Comment 25

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Henrik Skupin (:whimboo) [away 07/29 - 08/11] from comment #24)
> Comment on attachment 781717 [details] [diff] [review]
> collector support renaming (master) rev1.4
> 
> Review of attachment 781717 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just curious. Why has this patch not been exported with format-patch?

I've only ever generated patches using git diff, sorry. I can use format-patch in the future if preferred.

Comment 26

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Landed in:
https://github.com/mozilla/mozmill/commit/31dd1e0bafc815478c83b569a550216f8bcf0c37

Thanks Jonathan!