870384 - Improve Mutt python js-test module naming and structure for clarity

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Jonathan French (:jfrench)]

In the course of working on bug 858731, the following test appeared to be missing from the mutt/mutt/tests/python/js-tests/example.ini manifest.

test_module3.js

I've confirmed it runs fine, when added.

If appropriate, I will add it to the manifest.

If appropriate, I will rename ./example.ini to ./tests.ini, so it is consistent with the rest of the harness.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

We should move this ini file and all appropriate tests into a sub folder to have them all in one place. Renaming the ini file then sounds fine to me. Thanks for catching that Jonathan!

Comment 2

[Jonathan French (:jfrench)]

Cool. So how does this sound for structure.

js-tests/restart_endtest/ (unchanged)
js-tests/expectstack.js (unchanged)
js-tests/tests.ini (renamed - was "example.ini")

js-tests/modules/ (new)
js-tests/modules/test1.js (renamed - was "test_module1.js")
js-tests/modules/test2.js (renamed - was "test_module2.js")
js-tests/modules/test3.js (renamed - was "test_module3.js")
js-tests/modules/tests.ini (new)

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

(In reply to Jonathan French (:jfrench) from comment #2)
> js-tests/tests.ini (renamed - was "example.ini")

This ini file is only used for a cli test which actually even not uses it. We could call it tests-empty.ini. What do you think?

> js-tests/modules/ (new)
> js-tests/modules/test1.js (renamed - was "test_module1.js")
> js-tests/modules/test2.js (renamed - was "test_module2.js")
> js-tests/modules/test3.js (renamed - was "test_module3.js")
> js-tests/modules/tests.ini (new)

I think we should better bind js test files to their python tests and give them better names. As of now it is very hard to determine where they belong to.

Comment 4

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> (In reply to Jonathan French (:jfrench) from comment #2)
> > js-tests/tests.ini (renamed - was "example.ini")
> 
> This ini file is only used for a cli test which actually even not uses it.
> We could call it tests-empty.ini. What do you think?

Ya, I agree. test-empty, tests-placeholder, tests-null.ini, if it has to structurally be there. If the file can be empty, we could actually put a comment into this .ini instead, pointing to the python cli that actually drives these directories.

> 
> > js-tests/modules/ (new)
> > js-tests/modules/test1.js (renamed - was "test_module1.js")
> > js-tests/modules/test2.js (renamed - was "test_module2.js")
> > js-tests/modules/test3.js (renamed - was "test_module3.js")
> > js-tests/modules/tests.ini (new)
> 
> I think we should better bind js test files to their python tests and give
> them better names. As of now it is very hard to determine where they belong
> to.

Sure, if you have any suggestions I will use them. I had a look at each of the three tests; there's nothing really in each that appears to be unique enough to identify them from each other. But I will look at them again.

Perhaps naming the sub directory which will refer to the cli python driving it:

js-tests/cli/
js-tests/cli/(someTestName...).js
js-tests/cli/(someTestName...).js
js-tests/cli/(someTestName...).js
js-tests/cli/tests.ini

Comment 5

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #3)
> 
> > js-tests/modules/ (new)
> > js-tests/modules/test1.js (renamed - was "test_module1.js")
> > js-tests/modules/test2.js (renamed - was "test_module2.js")
> > js-tests/modules/test3.js (renamed - was "test_module3.js")
> > js-tests/modules/tests.ini (new)
> 
> I think we should better bind js test files to their python tests and give
> them better names. As of now it is very hard to determine where they belong
> to.

Looking at this further, I see that the parent py running these modules is ../usemozmill.py. So perhaps we could do something like

js-tests/unused.ini (formerly example.ini)
js-tests/expectStack.js
js-tests/modules/unused.ini (new)
js-tests/modules/usemozmillControllerOpen.js (formerly test_module1.js)
js-tests/modules/usemozmillExpectFail.js (formerly test_module2.js)
js-tests/modules/usemozmillExpectSkip.js (formerly test_module3.js)

What do you think?

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

(In reply to Jonathan French (:jfrench) from comment #4)
> > This ini file is only used for a cli test which actually even not uses it.
> > We could call it tests-empty.ini. What do you think?
> 
> Ya, I agree. test-empty, tests-placeholder, tests-null.ini, if it has to
> structurally be there. If the file can be empty, we could actually put a
> comment into this .ini instead, pointing to the python cli that actually
> drives these directories.

Sure.

> > > js-tests/modules/ (new)
> > > js-tests/modules/test1.js (renamed - was "test_module1.js")
> > > js-tests/modules/test2.js (renamed - was "test_module2.js")
> > > js-tests/modules/test3.js (renamed - was "test_module3.js")
> > > js-tests/modules/tests.ini (new)
> > 
> > I think we should better bind js test files to their python tests and give
> > them better names. As of now it is very hard to determine where they belong
> > to.
> 
> Sure, if you have any suggestions I will use them. I had a look at each of
> the three tests; there's nothing really in each that appears to be unique
> enough to identify them from each other. But I will look at them again.
> 
> Perhaps naming the sub directory which will refer to the cli python driving
> it:
> 
> js-tests/cli/
> js-tests/cli/(someTestName...).js
> js-tests/cli/(someTestName...).js
> js-tests/cli/(someTestName...).js
> js-tests/cli/tests.ini

Sounds fine. Even we would have to duplicate some of the test files which are used by different python tests. But that should be ok, given that those are testing specific behaviors and changing one of the files could affect another test or make it invalid.

So I'm totally up for this!

Comment 7

[Jonathan French (:jfrench)]

I've started mocking up the change locally. I'm proposing some slight modifications to the naming which I think may set a good generalized precedent for future tests which may be added.

Put simply, post proposed-change:

js-modules/
js-modules/endtestModules/endtestRestartModule.js (was "test1.js")
js-modules/endtestModules/endtestRestartShutdownModule.js (was "test2.js")
js-modules/endtestModules/test.ini

js-modules/tests-null.ini (was "example.ini")
js-modules/expectstackModule.js
js-modules/usemozmillControllerOpenModule.js (was "test_module1.js")
js-modules/usemozmillExpectFailModule.js (was "test_module2.js")
js-modules/usemozmillExpectSkipModule.js (was "test_module3.js")
js-modules/waitForPassFrameModule.js (was "test_waitFor_pass_frame.js")

And adjusting the related .py tests above this directory and in /cli, to load the new filenames. If we need 'duplicate' modules which are tied to other tests, this naming convention will also afford that. Sound good? Improvements?

Comment 8

[Jonathan French (:jfrench)]

(In reply to Jonathan French (:jfrench) from comment #7)
> js-modules/endtestModules/test.ini

that would be tests.ini plural, mentioned above.. name unchanged.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

(In reply to Jonathan French (:jfrench) from comment #7)
> js-modules/
> js-modules/endtestModules/endtestRestartModule.js (was "test1.js")
> js-modules/endtestModules/endtestRestartShutdownModule.js (was "test2.js")

We should not change the name of the test files here. The numbers are required by restart tests when we had individual files, so we should keep that. Putting them under such a folder seems to be fine.

> js-modules/tests-null.ini (was "example.ini")
> js-modules/expectstackModule.js
> js-modules/usemozmillControllerOpenModule.js (was "test_module1.js")
> js-modules/usemozmillExpectFailModule.js (was "test_module2.js")
> js-modules/usemozmillExpectSkipModule.js (was "test_module3.js")
> js-modules/waitForPassFrameModule.js (was "test_waitFor_pass_frame.js")

Please let all test filenames start with 'test'. I know there is another bug open to fix the current naming, but when we already do such a partial change, lets do it for those files.

Otherwise sounds good, yes.

Comment 10

[Jonathan French (:jfrench)]

Ok, I have the preliminary changes done and everything looks nice, mixedCase for the js tests, and `mutt testpy` run with the changes passes the same as clean master. Everything appears to be hooked up correctly.

Couple questions.

a) I'd like to make all the py test names one directory above, in mutt/mutt/tests/python, consistent:
eg. 
"test_testname.py"
- they are currently quite varied mix of naming conventions
- I could optionally open a separate bug for that change if you prefer

b) mutt/mutt/tests/python/js-modules/tests-null.ini contain the names of the tests (well, I have added the new names in, to make them tidy). But since this .ini is not used - should we have a completely empty file, or some variant of that? Or perhaps all the test names, with a comment in the .ini that it is unused? If there is a way to comment an ini.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

I think lets leave it empty. We might want to call it manifest_empty.ini.

Comment 12

[Jonathan French (:jfrench)]

Attachment: consistent mutt python module naming (master) rev1.0

Patch "consistent mutt python module naming (master) rev1.0" for Master. Twenty-three files modified, and in some cases renamed.

This represents work to make some beautiful, clean, consistent structure and naming conventions for the mutt python tests. Tested with Nightly 20130610031147.

The original bug, namely addressing the unused example.ini, has been addressed. That file is now manifest-empty.ini as directed, and has no contents.

In some cases, js-modules/ cli/ and py test names have been adjusted slightly, to be consistent and more clearly describe their purpose, when looking at the filename. Name changes were derived from the comments inside the test which describe its purpose.

An example, pysanity.py was renamed to test_framework_sanity.py, since based on the comment in the test that is what it does.

mixedCase was used for js tests in the js-modules/ directory. Former test_module(n).js were renamed according to their purpose.

test_(testname).py was used as the naming standard for the py tests.

Mutt tests run multiple times to confirm that the test results in my local branch with these changes are the same as the current clean master. And that all tests that are enabled, are being found during the run. 

I am making the assumption that the .pyc files in the local client are excluded from git.

It all results in a very nice looking tree.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 760592 [details] [diff] [review]
consistent mutt python module naming (master) rev1.0

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

In general this is really hard to review given that splinter shows the full files for removal and addition instead of only the rename/move. I think something I would like to see is a github branch diff between master and your integration branch. Would you mind doing that? Once all is fine we could export it to Bugzilla.

::: mutt/mutt/tests/python/cli/test_optionsandmanifest_exclusive.py
@@ +1,1 @@
> +#!/usr/bin/env python

The new file name reads very hard. Please keep the underlines between each word for the Python tests.

::: mutt/mutt/tests/python/cli/test_profile_relpath.py
@@ +1,1 @@
> +#!/usr/bin/env python

I would use the full name and not make abbreviations.

::: mutt/mutt/tests/python/js-modules/testUsemozmillControllerOpen.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Please take care of the exact camelcase usage. Also this is not bound to controller open but a test for the general Mozmill usage. So it should be something like test_useMozmillExpectPass.

::: mutt/mutt/tests/python/js-modules/testUsemozmillExpectFail.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Would test_useMozmillExpectFail be better here?

::: mutt/mutt/tests/python/tests.ini
@@ +7,2 @@
>  [test_bug690154.py]
> +[test_waitforpassframe.py]

please separate words or method names so it becomes test_waitFor_pass_frame.py

@@ +7,4 @@
>  [test_bug690154.py]
> +[test_waitforpassframe.py]
> +[test_endtest.py]
> +[test_loggerlistener.py]

Here it would be: test_logger_listener.py. Same applies to other file names.

Comment 14

[Jonathan French (:jfrench)]

Great, thanks for this info. I will make the changes described, and provide a diff of the rename mapping, with the next patch rev.

For reference, my naming all .js modules as testThingThing.js, was based on the mozmill-tests themselves? ie. there is no 'test_thingThing.js' convention used there. So I was being consistent with that. Should I deviate, and use test_thingThing.js in this repo?

For reference, my naming of all .py tests as test_thing_thing.py was loosely based on this information below
http://www.python.org/dev/peps/pep-0008/#package-and-module-names
http://www.python.org/dev/peps/pep-0008/#naming-conventions

Based on that, I was thinking of proposing test_wait_for_pass_frame.py to avoid camel case for the py files in the repo? But I see where you are going with using the _waitFor_. Let me know which you prefer. As the python folks themselves confirm "the naming conventions of Python's library are a bit of a mess", we will be at least more consistent than they are :)

Comment 15

[Jonathan French (:jfrench)]

Actually, looking at it changed, _waitfor_ does look better, so I've gone with that.

Comment 16

[Jonathan French (:jfrench)]

Attachment: rename mappings rev1.1

Here's a nice color coded grab of the commit window. It seems to be a nice way to view the name re-mappings. I've simply attached that screen grab for feedback.

I've left the .js tests as 'testThingThing.js', to be consistent with mozmill-tests. If they should deviate, and be 'test_thingThing.js', just reconfirm and I will change them.

After feedback, I will make tweaks as needed and upload a rev1.x patch.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 762262 [details]
rename mappings rev1.1

I think that looks fine. In the future you could also copy and paste this output into an attachment. :)

Comment 18

[Jonathan French (:jfrench)]

Attachment: consistent mutt python module naming (master) rev1.1

Patch "consistent mutt python module naming (master) rev1.1" for Master.

This makes all the renaming adjustments described in Comment 13, and adjusts the manifests and tests which open those modules, accordingly.

Ran mutt testpy (and testjs) multiple times to confirm that the test results in my local branch with these changes are the same as the current clean master. And that all tests that are enabled, are being found during the run.

Patch has been built in the new format-patch output format. The date beside the commit hash is interesting, but I am assuming that is expected.

Tested with Nightly 20130613031237. All tests pass where expected.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 762717 [details] [diff] [review]
consistent mutt python module naming (master) rev1.1

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

Looks good so far. Just some minor adjustments I would do to the file names as given inline. Once updated we can land the patch.

::: mutt/mutt/tests/python/cli/tests.ini
@@ +1,4 @@
>  [DEFAULT]
>  type = python
>  
> +[test_options_and_manifest_exclusive.py]

I would name it test_manifest_and_tests_exclusive.

::: mutt/mutt/tests/python/js-modules/restartTests/testEndtest/tests.ini
@@ +1,1 @@
> +[DEFAULT]

Can you please rename the parent folder to testEndTest?

::: mutt/mutt/tests/python/js-modules/testUseMozmillExpectFail.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I would name this file testUseMozmillTestFails.js

::: mutt/mutt/tests/python/js-modules/testUseMozmillExpectPass.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I would name this testUseMozmillTestPass.

::: mutt/mutt/tests/python/js-modules/testUseMozmillExpectSkip.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

I would name this file testUseMozmillTestSkip.js

::: mutt/mutt/tests/python/test_bug_690154.py
@@ +1,1 @@
> +#!/usr/bin/env python

Would you mind to give this file a real file name? That would make it better recognizable.

Comment 20

[Jonathan French (:jfrench)]

All the suggestions make good logical sense. Patch coming up shortly. 

I think I will go with a convention for the last bug file, as "test_bug_[nnnnnn]_[bug_description].py" so the first half of the filename for bugs will be consistent, and therefore searchable.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

No, please get rid of the bug number completely. It's present inside the test and we don't need it at this level.

Comment 22

[Jonathan French (:jfrench)]

Ok, will do.

Comment 23

[Jonathan French (:jfrench)]

Attachment: consistent mutt python module naming (master) rev1.2

Patch "consistent mutt python module naming (master) rev1.2" for Master.

This makes all minor adjustments described in Comment 19, and adjusts the manifests and tests which open those modules accordingly.

Re Comment 21, test_bug_690154.py has been renamed test_throw_global_exception.py

Ran mutt testpy (and testjs) to confirm that the test results in my local branch are the same as the current clean master. And that all tests that are enabled, are being found during the run. Patch is rebased to latest master.

Tested with Nightly 20130617031112. All tests pass where expected.

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 764151 [details] [diff] [review]
consistent mutt python module naming (master) rev1.2

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

::: mutt/mutt/tests/python/test_end_test.py
@@ +15,5 @@
> +        manifest = manifestparser.TestManifest(
> +            manifests=[os.path.join(os.path.dirname(os.path.abspath(__file__)),
> +                                    "js-modules",
> +                                    "restartTests",
> +                                    "testEndtest",

This should be testEndTest. As of now this causes the test to fail. Please execute a run next time before submitting the patch. :) I will fix it before landing.

Comment 25

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Landed on master:
https://github.com/mozilla/mozmill/commit/db556c4953c5340b3ed01d4b5fc0d9a8ef8c8017

Comment 26

[Jonathan French (:jfrench)]

Thanks, appreciated. A useful reminder to me for future restructuring work like this on a windows platform, that the mutt run environment on windows is case insensitive. In my case, the test was passing in mutt; since /testEndTest = /testEndtest = /testENDTEST, and so forth, on windows.

Mutt was happily passing that test on my platform, so that required character change was not exposed.

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

I backed this out due to the vanishing of all the tests. Jonathan, can you please make the little change as requested and upload a new version? Being able to run git am directly would not have caused this problem. :/

Comment 28

[Jonathan French (:jfrench)]

Yup, will do.

Comment 29

[Jonathan French (:jfrench)]

Attachment: consistent mutt python module naming (master) rev1.3

Patch "consistent mutt python module naming (master) rev1.3" for Master.

This allows for the overlay on the backout, and updates test_end_test.py with testEndTest, as per Comment 24 at the same time. Updated the Commit comment to be consistent with Henrik's originally landed patch.

No mutt test run done for expediency, just diff'd the earlier rev1.2 patch against this rev1.3 to ensure the only change were those described above.

Comment 30

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Landed the full patch on master:
https://github.com/mozilla/mozmill/commit/7a98161d79450b1973c1b51907de056e109d62a7