643562 - "FAIL: Doctest: xpi.md" on Linux with fresh checkout

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

Something very strange is happening on my Linux VM.  When I check out a fresh copy of the code, activate it, and run testcfx, I get a test failure:

--------------------------------------------------------------------------------
myk@myk:~$ git clone [e-mail]:mozilla/addon-sdk.git
Initialized empty Git repository in /home/myk/addon-sdk/.git/
remote: Counting objects: 12091, done.
remote: Compressing objects: 100% (6479/6479), done.
remote: Total 12091 (delta 6022), reused 10933 (delta 5090)
Receiving objects: 100% (12091/12091), 4.83 MiB | 2.10 MiB/s, done.
Resolving deltas: 100% (6022/6022), done.
myk@myk:~$ cd addon-sdk
myk@myk:~/addon-sdk$ source bin/activate
Welcome to the Add-on SDK. Run 'cfx docs' for assistance.
(addon-sdk)myk@myk:~/addon-sdk$ cfx testcfx
...................................................................F...................................
======================================================================
FAIL: Doctest: xpi.md
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/usr/lib/python2.6/doctest.py", line 2152, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for xpi.md
  File "/home/myk/addon-sdk/static-files/md/dev-guide/module-development/xpi.md", line 0

----------------------------------------------------------------------
File "/home/myk/addon-sdk/static-files/md/dev-guide/module-development/xpi.md", line 120, in xpi.md
Failed example:
    document_dir('xpi-output')
Differences (unified diff with -expected +actual):
    @@ -14,8 +14,4 @@
     resources/guid-aardvark-lib/:
     <BLANKLINE>
    -resources/guid-aardvark-lib/ignore_me:
    -  The docs processor should tolerate (by ignoring) random non-.js files in lib
    -  directories, such as those left around by editors, version-control systems,
    -  or OS metadata like .DS_Store . This file exercises that tolerance.
     resources/guid-aardvark-lib/main.js:
       exports.main = function(options, callbacks) {
    @@ -23,4 +19,8 @@
         callbacks.quit();
       };
    +resources/guid-aardvark-lib/ignore_me:
    +  The docs processor should tolerate (by ignoring) random non-.js files in lib
    +  directories, such as those left around by editors, version-control systems,
    +  or OS metadata like .DS_Store . This file exercises that tolerance.
     resources/guid-aardvark-lib/surprise.js/ignore_me_too:
       The docs processor should also ignore directories named *.js, and their
    @@ -34,65 +34,65 @@
     harness-options.json:
       {
    -   "loader": "resource://guid-api-utils-lib/loader.js",
    -   "main": "main",
    +   "loader": "resource://guid-api-utils-lib/loader.js",
    +   "main": "main",
        "manifest": {
         "resource://guid-aardvark-lib/main.js": {
    -     "chrome": false,
    -     "e10s-adapter": null,
    -     "hash": "a592cf3cf924f2c77e0728d97131138fcb7495c77f5202ac55c2e0c77ef903c2",
    -     "name": "main",
    -     "packageName": "aardvark",
    +     "chrome": false,
    +     "e10s-adapter": null,
    +     "hash": "a592cf3cf924f2c77e0728d97131138fcb7495c77f5202ac55c2e0c77ef903c2",
    +     "name": "main",
    +     "packageName": "aardvark",
          "requires": {
           "bar-module": {
            "url": "resource://guid-barbeque-lib/bar-module.js"
           }
    -     },
    -     "sectionName": "lib",
    +     },
    +     "sectionName": "lib",
          "zipname": "resources/guid-aardvark-lib/main.js"
    -    },
    +    },
         "resource://guid-api-utils-lib/loader.js": {
    -     "chrome": false,
    -     "e10s-adapter": null,
    -     "hash": "efac9dc700a56e693ac75ab81955c11e6874ddc83d92c11177d643601eaac346",
    -     "name": "loader",
    -     "packageName": "api-utils",
    -     "requires": {},
    -     "sectionName": "lib",
    +     "chrome": false,
    +     "e10s-adapter": null,
    +     "hash": "efac9dc700a56e693ac75ab81955c11e6874ddc83d92c11177d643601eaac346",
    +     "name": "loader",
    +     "packageName": "api-utils",
    +     "requires": {},
    +     "sectionName": "lib",
          "zipname": "resources/guid-api-utils-lib/loader.js"
    -    },
    +    },
         "resource://guid-barbeque-lib/bar-module.js": {
    -     "chrome": false,
    -     "e10s-adapter": null,
    -     "hash": "2515f8623e793571f1dffc4828de14a00a3da9be666147f8cebb3b3f1929e4d6",
    -     "name": "bar-module",
    -     "packageName": "barbeque",
    -     "requires": {},
    -     "sectionName": "lib",
    +     "chrome": false,
    +     "e10s-adapter": null,
    +     "hash": "2515f8623e793571f1dffc4828de14a00a3da9be666147f8cebb3b3f1929e4d6",
    +     "name": "bar-module",
    +     "packageName": "barbeque",
    +     "requires": {},
    +     "sectionName": "lib",
          "zipname": "resources/guid-barbeque-lib/bar-module.js"
         }
    -   },
    -   "packageData": {},
    +   },
    +   "packageData": {},
        "resourcePackages": {
    -    "guid-aardvark-lib": "aardvark",
    -    "guid-api-utils-lib": "api-utils",
    +    "guid-aardvark-lib": "aardvark",
    +    "guid-api-utils-lib": "api-utils",
         "guid-barbeque-lib": "barbeque"
    -   },
    +   },
        "resources": {
         "guid-aardvark-lib": [
    -     "resources",
    +     "resources",
          "guid-aardvark-lib"
    -    ],
    +    ],
         "guid-api-utils-lib": [
    -     "resources",
    +     "resources",
          "guid-api-utils-lib"
    -    ],
    +    ],
         "guid-barbeque-lib": [
    -     "resources",
    +     "resources",
          "guid-barbeque-lib"
         ]
    -   },
    +   },
        "rootPaths": [
    -    "resource://guid-api-utils-lib/",
    -    "resource://guid-barbeque-lib/",
    +    "resource://guid-api-utils-lib/",
    +    "resource://guid-barbeque-lib/",
         "resource://guid-aardvark-lib/"
        ]


----------------------------------------------------------------------
Ran 103 tests in 3.750s

FAILED (failures=1)
--------------------------------------------------------------------------------

Some of those changed lines don't show any changes, however!  And I don't see the failure on my existing clone of the repository on the same VM nor on fresh clones on Mac and Windows systems.

Does anyone else see this problem?  If we can reproduce it consistently, then it's probably a release blocker.

Note: I've looked for whitespace and line ending changes, but the lines appear to be identical, including whitespace.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Reverting changeset 345b68ec7e443f3aef85dc65b285a22ab229de75 <https://github.com/mozilla/addon-sdk/commit/345b68ec7e443f3aef85dc65b285a22ab229de75> unfails the test, so it looks like that's the cause of the problem.

Comment 2

[Will Bamberg [:wbamberg]]

Yeah, I see this as well on a fresh checkout on Linux.

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I saw this (and ignored it) while fixing the doctest fail that I had caused in bug 642970, in Ubuntu.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to comment #0)

> Some of those changed lines don't show any changes, however!  
Maybe github doesn't care about line-endings, but Linux does?

Comment 5

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #4)
> (In reply to comment #0)
> 
> > Some of those changed lines don't show any changes, however!  
> Maybe github doesn't care about line-endings, but Linux does?

No, I checked the line endings, and they're the same on the changed lines.  A bit more digging suggests that only the first set of changes is pertinent; the others appear to be test harness misreportage.  So the issue is:

    -resources/guid-aardvark-lib/ignore_me:
    -  The docs processor should tolerate (by ignoring) random non-.js files in
lib
    -  directories, such as those left around by editors, version-control
systems,
    -  or OS metadata like .DS_Store . This file exercises that tolerance.
     resources/guid-aardvark-lib/main.js:
       exports.main = function(options, callbacks) {
    @@ -23,4 +19,8 @@
         callbacks.quit();
       };
    +resources/guid-aardvark-lib/ignore_me:
    +  The docs processor should tolerate (by ignoring) random non-.js files in
lib
    +  directories, such as those left around by editors, version-control
systems,
    +  or OS metadata like .DS_Store . This file exercises that tolerance.

In other words, ignore_me appears after main.js, whereas the test expects ignore_me to appear before main.js.  My two clones on my Linux VM behave differently, however.  One fails this test, while the other one passes it.

I'm becoming more skeptical that this is a blocker, but I'd love to have a better sense of why this is happening.

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

> One fails this test, while the other one passes it.
If you switch the order around to what it expects, do your VMs flip behavior accordingly? (The one that passed it previously fails, the one that failed passes?)

Comment 7

[Alexandre Poirot [:ochameau]]

I took a look at this bug without much success ...

On windows, I only have errors around harness-options.json.

I don't have any diffs on ignore_me, but I would easily understand random file
orders in packaging.py:_get_files_in_dir function that use os.listdir.
But then I don't see why the Brian commit change in that already random order!

Comment 8

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> > One fails this test, while the other one passes it.
> If you switch the order around to what it expects, do your VMs flip behavior
> accordingly? (The one that passed it previously fails, the one that failed
> passes?)

Yes, both clones are consistent.  One expects the files in one order, the other expects them in the opposite order.  So changing the test consistently results in a failure on one of the clones and not the other.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: explicitly sorts files before checking them

(In reply to comment #7)
> I don't have any diffs on ignore_me, but I would easily understand random file
> orders in packaging.py:_get_files_in_dir function that use os.listdir.
> But then I don't see why the Brian commit change in that already random order!

os.listdir definitely seems like a candidate, although I'm seeing a different execution path...

xpi.md calls test_xpi.py:document_dir('xpi-output'), which calls create_xpi('test-xpi.xpi'), which calls xpi.py:build_xpi(), which calls os.walk(), which relies on os.listdir(), which, as you suggest, returns files in an "arbitrary order" <http://docs.python.org/library/os.html#os.listdir>.

Here's a patch that explicitly sorts the file list before testing it.  Requesting review from Brian, who knows this code best and should be back at work tomorrow (Tuesday) to review it.

Now that I understand this better, I don't think it's a release blocker, as much as I loathe shipping with test failures (even intermittent ones).  Nevertheless, since we have a fix in hand, and since I'm not going to spin a second RC until tomorrow anyway at this point, I'll take this fix for RC2 if we get it reviewed and committed in time.

Comment 10

[Brian Warner [:warner :bwarner]]

Comment on attachment 520848 [details] [diff] [review]
patch v1: explicitly sorts files before checking them

The patch looks good to me, although personally I'd use the shorter sorted() form, like this:

  for name in sorted(zip.namelist()):

Those doctests are frustrating. They're not sensitive to whitespace, but if there are any non-whitespace mismatches, the complaining diff it prints out *does* include whitespace, making it awfully hard to track down what really did mismatch. After this release, I'm seriously thinking of just ripping out this test entirely: it has to be updated constantly, takes a long time to fix each time, and I'm not really convinced of the value it provides.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #10)
> The patch looks good to me, although personally I'd use the shorter sorted()
> form, like this:
> 
>   for name in sorted(zip.namelist()):

Ah, thanks!  I was bummed to learn that .sort() didn't return the sorted list when digging in the Python docs; this is exactly the syntax I was looking for, and this is what I checked in.

https://github.com/mozilla/addon-sdk/commit/d7d7d495f9feb5046b4d49bcfb3f9de28bba3638

Cherry-pick to 1.0b4 branch:

https://github.com/mozilla/addon-sdk/commit/cbc1f81859f44c6d56060c0a7b545f0b02f404e0


> Those doctests are frustrating. They're not sensitive to whitespace, but if
> there are any non-whitespace mismatches, the complaining diff it prints out
> *does* include whitespace, making it awfully hard to track down what really did
> mismatch. After this release, I'm seriously thinking of just ripping out this
> test entirely: it has to be updated constantly, takes a long time to fix each
> time, and I'm not really convinced of the value it provides.

+1