1621244 - mach test fails to find head files when mach xpcshell-test works

Status: RESOLVED FIXED

(Testing :: General, defect, P2)

Description

[Florian Quèze [:florian]]

When running ./mach test services/settings/test/unit/test_attachments_downloader.js it fails for me with this error:

ERROR Traceback (most recent call last):
  File "/Users/florian/buildhg/mozilla/testing/xpcshell/runxpcshelltests.py", line 186, in run
    self.run_test()
  File "/Users/florian/buildhg/mozilla/testing/xpcshell/runxpcshelltests.py", line 662, in run_test
    self.command.extend(self.buildCmdHead())
  File "/Users/florian/buildhg/mozilla/testing/xpcshell/runxpcshelltests.py", line 419, in buildCmdHead
    headfiles = self.getHeadFiles(self.test_object)
  File "/Users/florian/buildhg/mozilla/testing/xpcshell/runxpcshelltests.py", line 449, in getHeadFiles
    return list(sanitize_list(headlist, 'head'))
  File "/Users/florian/buildhg/mozilla/testing/xpcshell/runxpcshelltests.py", line 441, in sanitize_list
    raise Exception('%s file does not exist: %s' % (kind, path))
Exception: head file does not exist: /Users/florian/buildhg/mozilla/obj-artifact/_tests/xpcshell/services/common/tests/unit/head_global.js

Starting the same test with ./mach xpcshell-test services/settings/test/unit/test_attachments_downloader.js worked just fine.

In case this matters, I was running this on an artifact build.
After starting the test with mach xpcshell-test at least once, the following attempts to start it using mach test work fine.

Comment 1

[Ricky Stewart]

I can reproduce this at HEAD with an artifact build. So mach test delegates to the same codepath that mach xpcshell-test runs, which is as I would expect. I can only assume that that delegation involves calling mach xpcshell-test with slightly different arguments that over the course of the call to xpcshell-test result in the error shown here; unfortunately I have no experience with this though, so I can't say exactly what the problem is.

Geoffrey, do you have an idea what the issue would be here, and if not do you have an idea of someone who might be a better owner for the bug?

Comment 2

[Geoff Brown [:gbrown]]

That's really strange, and I don't know where the issue is, but I have tracked down problems like this before -- re-assign to me, if you like.

Comment 3

[Ricky Stewart]

Thanks, let me know what you find.

Comment 4

[Geoff Brown [:gbrown]]

From the perspective of the options seen by runTests(), I find this difference:
./mach xpcshell-test (works)
{'testPaths': ['services/settings/test/unit/test_attachments_downloader.js'], 'manifest': '/home/gbrown/objdirs/firefox/_tests/xpcshell/xpcshell.ini'}
./mach test (fails)
{'test_file': 'all', 'testPaths': [], 'manifest': <manifestparser.manifestparser.TestManifest object at 0x7f427fda92d0>}

That is, mach test creates and passes a manifest where mach xpcshell-test passes paths to the test file and master manifest file.

Comment 5

[Geoff Brown [:gbrown]]

(In reply to Geoff Brown [:gbrown] from comment #4)

From the perspective of the options seen by runTests(), I find this difference:

Actually, that seems to be irrelevant.

Comment 6

[Geoff Brown [:gbrown]]

install_tests() is more important:
https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/testing/xpcshell/mach_commands.py#230

./mach xpcshell-test services/settings/test/unit/test_attachments_downloader.js calls install_tests(None) and populates services/common if needed; ./mach test services/settings/test/unit/test_attachments_downloader.js calls install_tests() with test_objects ==:

[{'head': '../../../common/tests/unit/head_global.js ../../../common/tests/unit/head_helpers.js', 
'name': 'test_attachments_downloader.js', 
'support-files': '\ntest_remote_settings_signatures/** test_attachments_downloader/**', 
'tags': 'remote-settings', 'here': u'/home/gbrown/objdirs/firefox/_tests/xpcshell/services/settings/test/unit',
'manifest': u'/home/gbrown/src/services/settings/test/unit/xpcshell.ini',
'firefox-appdir': 'browser', u'file_relpath': u'services/settings/test/unit/test_attachments_downloader.js', 
'path': u'/home/gbrown/objdirs/firefox/_tests/xpcshell/services/settings/test/unit/test_attachments_downloader.js',
'manifest_relpath': u'services/settings/test/unit/xpcshell.ini',
u'dir_relpath': u'services/settings/test/unit', 
u'srcdir_relpath': u'services/settings/test/unit/test_attachments_downloader.js', 
u'flavor': u'xpcshell', 
'relpath': u'services/settings/test/unit/test_attachments_downloader.js'}]

and does not populate services/common at all.

Comment 7

[Geoff Brown [:gbrown]]

https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/python/mozbuild/mozbuild/testing.py#216
appears to make no attempt to install 'head' files. Also, it doesn't use relpath (etc) as a backup for file_relpath.

Comment 8

[Geoff Brown [:gbrown]]

...but the SupportFilesConverter does handle head files (usually). And I can't find any practical problem with the relpath/file_relpath issue.

Comment 9

[Geoff Brown [:gbrown]]

However, when a head file is not in the same directory as the manifest, it is ignored (not installed), and that appears to be intentional

https://searchfox.org/mozilla-central/rev/4d2a9d5dc8f0e65807ee66e2b04c64596c643b7a/python/mozbuild/mozbuild/testing.py#177-182

                        # If it's not a support file, we ignore it.
                        # This preserves old behavior so things like
                        # head files doesn't get installed multiple
                        # times.

...from bug 1242051 (but I don't see specific commentary on this issue in the bug).

This (ignoring head files) seems wrong to me, and I don't have much concern for installing multiple times -- these are just symlinks, and head files outside of the manifest directory are an important case, but not a common case.

Comment 10

[Geoff Brown [:gbrown]]

Attachment: Bug 1621244 - Ensure test head files are installed, even when in another directory; r=

The effect of the deleted code has been to not install required head files
when those head files are not in the same directory as the test's manifest;
that seems wrong. I am slightly concerned about the comment justifying the
short-cut, but I cannot find any ill effect from removing the code, and doing
so allows test_attachments_downloader.js to run with either xpcshell-test or test.

Comment 11

[Geoff Brown [:gbrown]]

Attachment: Bug 1621244 - Ensure all test files are installed, even when running only a subset; r=

Most (all?) mach xpcshell-test calls result in calling install_tests(None) while
most mach test calls for the same test/directory result in calling install_tests()
with a collection of test objects. Providing test objects allows install_tests()
to optimize which tests are installed, but there have been several recent bugs
that appear to be related to that optimization. Let's rely less on that optimization
and make things consistent between test/xpcshell-test. (There's a parallel
consideration for test vs mochitest.)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1687bce0a804
Ensure test head files are installed, even when in another directory; r=bc

Comment 13

[Geoff Brown [:gbrown]]

:bc noticed that 'mach xpcshell-test --this-chunk=1 --total-chunks=100' fails with the first patch (comment 12); I've requested backout.

ValueError: Item already in manifest: xpcshell/devtools/client/shared/test/shared-redux-head.js

  File "/home/gbrown/src/testing/xpcshell/mach_commands.py", line 230, in run_xpcshell_test
    driver.install_tests()
  File "/home/gbrown/src/python/mozbuild/mozbuild/controller/building.py", line 1393, in install_tests
    '_tests')
  File "/home/gbrown/src/python/mozbuild/mozbuild/testing.py", line 191, in install_test_files
    manifest |= InstallManifest(harness_files_manifest)
  File "/home/gbrown/src/python/mozbuild/mozpack/manifests.py", line 206, in __ior__
    self.add_entries_from(other)
  File "/home/gbrown/src/python/mozbuild/mozpack/manifests.py", line 365, in add_entries_from
    self._add_entry(new_dest, new_entry)
  File "/home/gbrown/src/python/mozbuild/mozpack/manifests.py", line 335, in _add_entry
    raise ValueError('Item already in manifest: %s' % dest)

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc0e41f2650c
Backed out changeset 1687bce0a804 per gbrown's request on riot CLOSED TREE

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7da03ba6bfcc
Ensure all test files are installed, even when running only a subset; r=bc

Comment 16

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/7da03ba6bfcc