1117543 - running xpcshell-tests fails on linux.

Status: NEW

(MailNews Core :: Testing Infrastructure, defect)

Description

[Suyash Agarwal (:sshagarwal)]

Running |make xpcshell-tests| fails on mailnews/

Error:

/home/user/comm-central/thunderbird-binary/_virtualenv/bin/python -u /home/user/comm-central/mozilla/config/pythonpath.py \
  -I./build \
  -I/home/user/comm-central/mozilla/build \
  -I./_tests/mozbase/mozinfo \
  /home/user/comm-central/mozilla/testing/xpcshell/runxpcshelltests.py \
  --manifest=./_tests/xpcshell/xpcshell.ini \
  --build-info-json=./mozinfo.json \
  --no-logfiles \
  --test-plugin-path='dist/plugins' \
  --tests-root-dir=/home/user/comm-central/thunderbird-binary/_tests/xpcshell \
  --testing-modules-dir=/home/user/comm-central/thunderbird-binary/_tests/modules \
          --symbols-path=dist/crashreporter-symbols \
  --test-path='mailnews/addrbook/test/unit/test_mailList1.js'  \
  /home/user/comm-central/thunderbird-binary/dist/bin/xpcshell
Traceback (most recent call last):
  File "/home/user/comm-central/mozilla/config/pythonpath.py", line 56, in <module>
    main(sys.argv[1:])
  File "/home/user/comm-central/mozilla/config/pythonpath.py", line 48, in main
    execfile(script, frozenglobals)
  File "/home/user/comm-central/mozilla/testing/xpcshell/runxpcshelltests.py", line 1495, in <module>
    main()
  File "/home/user/comm-central/mozilla/testing/xpcshell/runxpcshelltests.py", line 1491, in main
    if not xpcsh.runTests(args[0], testdirs=args[1:], **options.__dict__):
  File "/home/user/comm-central/mozilla/testing/xpcshell/runxpcshelltests.py", line 1226, in runTests
    test_object['id'] = self.makeTestId(test_object)
  File "/home/user/comm-central/mozilla/testing/xpcshell/runxpcshelltests.py", line 1002, in makeTestId
    path = test_object[relpath_key].replace('\\', '/');
KeyError: 'relpath'
make: *** [xpcshell-tests] Error 1

Comment 1

[Suyash Agarwal (:sshagarwal)]

Attachment: xpcshell-fix

Can it be a possible regression from
http://hg.mozilla.org/mozilla-central/rev/df04db0326a3 ?

This patch makes |make xpcshell-tests| work for me.

Comment 2

[:aceman]

Comment on attachment 8543665 [details] [diff] [review]
xpcshell-fix

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

I confirm this problem happening for me for some weeks now. I also confirm the patch works for me. However I am not sure about the change as maybe the "relpath" is still needed for some cases. Maybe it needs to be file_relpath OR relpath OR path ? I do not see where the *relpath variables are even set.

Comment 3

[Andrew Halberstadt [:ahal]]

Comment on attachment 8543665 [details] [diff] [review]
xpcshell-fix

I'm not really sure why this is the case so I'll leave it to gps for now.. though I can investigate if he is also unsure.

Out of curiosity, is |mach xpcshell-test| also broken?

Comment 4

[:aceman]

Yes, see bug 934170. "make xpcshell-tests" was the reliable method for us for a long time so we need to quickly make it usable again. Mach is an unreliable secondary option for anything for now.

Comment 5

[Joshua Cranmer [:jcranmer]]

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> Out of curiosity, is |mach xpcshell-test| also broken?

For comm-central, mach xpcshell-test has never worked. Partially because some code decides that xpcshell-test in mach doesn't need to run unless you're building Firefox, Fennec, or B2G, and partially because it's upset at the lack of a CLOBBER file in comm-central.

Comment 6

[Andrew Halberstadt [:ahal]]

(In reply to Joshua Cranmer [:jcranmer] from comment #5)
> (In reply to Andrew Halberstadt [:ahal] from comment #3)
> > Out of curiosity, is |mach xpcshell-test| also broken?
> 
> For comm-central, mach xpcshell-test has never worked. Partially because
> some code decides that xpcshell-test in mach doesn't need to run unless
> you're building Firefox, Fennec, or B2G

If you're talking about the conditions stuff, then we should add a thunderbird condition [1]! That feature was never meant to exclude platforms, but rather to prevent confusion for people trying to run certain commands from inappropriate contexts. The CLOBBER file problem I wouldn't know what to do about.

[1] http://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/base.py#640

Comment 7

[Gregory Szorc [:gps]]

(In reply to :aceman from comment #4)
> Yes, see bug 934170. "make xpcshell-tests" was the reliable method for us
> for a long time so we need to quickly make it usable again. Mach is an
> unreliable secondary option for anything for now.

The ship has sailed: mach is the future. Make targets for running tests have been and will continue to be deprecated. Please file bugs (like this one) against mach commands if something is broken.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8543665 [details] [diff] [review]
xpcshell-fix

This feels like something chmanchester knows about.

On a preliminary look, not all entries in all-tests.json have "relpath" entries. That's a bit odd and seems like a bug on its own.

This patch earns r- because it is substituting a relative path with an absolute path. And that will almost certainly do bad things to the identifier. I think "name" would be a better choice as a replacement for "relpath." Again, I'm not sure why "relpath" isn't defined. Perhaps we should get file_relpath, relpath, name?

Comment 9

[:aceman]

Comment on attachment 8543665 [details] [diff] [review]
xpcshell-fix

Just to remind that this bug still exists and the patch is needed to get any xpcshell tests working (on Linux), even WITH mach.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8543665 [details] [diff] [review]
xpcshell-fix

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

::: testing/xpcshell/runxpcshelltests.py
@@ +996,5 @@
>  
>      def makeTestId(self, test_object):
>          """Calculate an identifier for a test based on its path or a combination of
>          its path and the source manifest."""
> +        relpath_key = 'file_relpath' if 'file_relpath' in test_object else 'path'

I can mostly just echo gps' feedback in comment 8.

My recollection is that 'file_relpath' was the key to use when running locally, and 'relpath' was the correct key in automation, so this change would break our logging in automation. A thing to investigate would be why we don't have a srcdir relative path available in this test object.

Comment 11

[:aceman]

(In reply to Gregory Szorc [:gps] from comment #8)
> On a preliminary look, not all entries in all-tests.json have "relpath"
> entries. That's a bit odd and seems like a bug on its own.
> 
> This patch earns r- because it is substituting a relative path with an
> absolute path. And that will almost certainly do bad things to the
> identifier. I think "name" would be a better choice as a replacement for
> "relpath." Again, I'm not sure why "relpath" isn't defined. Perhaps we
> should get file_relpath, relpath, name?

jcranmer do you know what is meant here? Is this something in the build config that creates this all-tests.json file?

Comment 12

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #11)
> (In reply to Gregory Szorc [:gps] from comment #8)
> > On a preliminary look, not all entries in all-tests.json have "relpath"
> > entries. That's a bit odd and seems like a bug on its own.
> > 
> > This patch earns r- because it is substituting a relative path with an
> > absolute path. And that will almost certainly do bad things to the
> > identifier. I think "name" would be a better choice as a replacement for
> > "relpath." Again, I'm not sure why "relpath" isn't defined. Perhaps we
> > should get file_relpath, relpath, name?
> 
> jcranmer do you know what is meant here? Is this something in the build
> config that creates this all-tests.json file?

It's created by the mozbuild backend. "relpath" is probably being set by the manifestdestiny parser, so like gps, I'd be confused as to why it isn't being retrieved.

Comment 13

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #9)
> Just to remind that this bug still exists and the patch is needed to get any
> xpcshell tests working (on Linux), even WITH mach.

I'm confused. e.g. this works for me:
 cd $objdir
 mach xpcshell-test mailnews/base/test/unit/test_accountMigration.js

What exactly isn't working? That you shouldn't have to go to the objdir?

Comment 14

[:aceman]

There is no mach in $objdir.
I need to do ../mozilla/mach in there.

So I do "../mozilla/mach xpcshell-test mailnews" and get this if I do not use the patch:
File "TB-hg/mozilla/testing/xpcshell/mach_commands.py", line 136, in _run_xpcshell_harness
    result = xpcshell.runTests(**filtered_args)
  File "TB-hg/mozilla/testing/xpcshell/runxpcshelltests.py", line 1204, in runTests
    self.buildTestList(test_tags, testPaths)
  File "TB-hg/mozilla/testing/xpcshell/runxpcshelltests.py", line 820, in buildTestList
    self.alltests = mp.active_tests(filters=filters, **mozinfo.info)
  File "TB-hg/mozilla/testing/mozbase/manifestparser/manifestparser/manifestparser.py", line 749, in active_tests
    return list(tests)
  File "TB-hg/mozilla/testing/mozbase/manifestparser/manifestparser/filters.py", line 356, in __call__
    for test in tests:
  File "TB-hg/mozilla/testing/mozbase/manifestparser/manifestparser/filters.py", line 53, in fail_if
    for test in tests:
  File "TB-hg/mozilla/testing/mozbase/manifestparser/manifestparser/filters.py", line 41, in run_if
    for test in tests:
  File "TB-hg/mozilla/testing/mozbase/manifestparser/manifestparser/filters.py", line 30, in skip_if
    if tag in test and parse(test[tag], **values):
  File "TB-hg/mozilla/testing/mozbase/manifestparser/manifestparser/expression.py", line 281, in parse
    return ExpressionParser(text, values).parse()
  File "TB-hg/mozilla/testing/mozbase/manifestparser/manifestparser/expression.py", line 267, in parse
    raise ParseError("could not parse: %s; variables: %s" % (self.text, self.valuemapping))

Comment 15

[:aceman]

Ah, this is the top of the log:
From _tests: Kept 37467 existing; Added/updated 0; Removed 0 files and 0 directories.
Error running mach:

    ['xpcshell-test', 'mailnews']

The error occurred in code that was called by the mach command. This is either
a bug in the called code itself or in the way that mach is calling it.

You should consider filing a bug for this issue.

If filing a bug, please include the full output of mach, including this error
message.

The details of the failure are as follows:

ParseError: could not parse: (toolkit == 'android' || toolkit == 'gonk') || ((os == "win" && os_version == "5.1") ); variables: {'topsrcdir': u'/var/SSD/TB-hg/mozilla', 'pgo': False, 'tool
kit': u'gtk3', 'webm': True, 'buildapp': u'../mail', 'crashreporter': False, 'addon_signing': False, 'platform_guess': None, 'appname': u'thunderbird', 'mozconfig': u'/var/SSD/TB-hg/.mozco
nfig', 'has_sandbox': True, 'telemetry': False, 'os_version': StringVersion (''), 'version': ' ', 'datareporting': False, 'buildtype_guess': u'debug', 'hasNode': False, 'bits': 32, 'bin_su
ffix': u'', 'wave': True, 'healthreport': False, 'release_build': False, 'asan': False, 'tests_enabled': True, 'linux_distro': '', 'official': False, 'tsan': False, 'debug': True, 'os': u'
linux', 'processor': u'x86'}

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #14)
> There is no mach in $objdir.
> I need to do ../mozilla/mach in there.

FWIW, I've just got the mozilla mach symlinked system wide.

Comment 17

[:aceman]

OK, I take back that this patch fixes the tests for me. Even with the patch the tests failed for me. I then found out the string in the log can't be parsed because linux_distro and os_version is empty for me. I looked at why that is and I found out that platform.linux_distribution() call in mozinfo.py (using the platform.py from python base install) returns empty strings. Investigating that, I found out the way that function determines the distro and version is quite verbose, just iterating all files in /etc and determining if they match a pattern. I have /etc not readable to the non-root users, so that fails...

Gps, should I file that as a bug and can it be worked around in mozinfo or somewhere else in xpcshell? Does the system really need to know the exact distro, when it already knows the OS is Linux?

Comment 18

[Gregory Szorc [:gps]]

I /think/ the "distro" attribute in mozinfo is unused and can be deleted. However, manifests like testing/web-platform/meta/XMLHttpRequest/responsexml-media-type.htm.ini actually do use the "version" attribute and contain references to Linux. Also "version" is the concatenation of 2 of the return values from platform.linux_distribution(). So, yes, mozinfo actually needs the results of platform.linux_distribution() to accurately run tests.

We /could/ make mozinfo treat platform.linux_distribution() as optional. However, this terrifies me because if we ever introduce a bug there, it could lead to us not executing tests in automation because various mozinfo attributes aren't defined properly. This fear could be reduced by introducing a configure check for platform.linux_distribution() validity and requiring a configure flag to allow empty values. A bit more complicated, but at least it makes the foot gun an explicit choice rather than a silent failure.

Comment 19

[:aceman]

Ok, I have found some more info about this code in platform.py and have split this issue into bug 1212502.

To return back to the issue of this bug. I have tried it now and I DO NOT need to apply this patch to run xpcshell-tests with mach nor make.

Suyash, is this patch still necessary for you?

Comment 20

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #19)
> Ok, I have found some more info about this code in platform.py and have
> split this issue into bug 1212502.
> 
> To return back to the issue of this bug. I have tried it now and I DO NOT
> need to apply this patch to run xpcshell-tests with mach nor make.
> 
> Suyash, is this patch still necessary for you?

No, I am able to run xpcshell-tests on my machine without any patch :)