Closed Bug 1283919 Opened 4 years ago Closed 4 years ago

Improve test package archiver for collecting files from directories referenced by a root manifest

Categories

(Firefox Build System :: General, defect)

49 Branch
defect
Not set

Tracking

(firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

Attachments

(1 file, 1 obsolete file)

Right now we still have to use make targets for test package archiving when tests are spread around in different components:

https://dxr.mozilla.org/mozilla-central/source/testing/testsuite-targets.mk#334

I would like to improve the test package archiver so it can collect all the referenced test files from listed manifests. With that we could drastically reduce the make files, and possibly allow to run more tests from the src tree.
Summary: Improve test package archiver for test files collection → Improve test package archiver for collecting test files from different directories
So that doesn't seem to be too complicated. It's just work which would need a bit of time. I will get started on this now.

I had a first look at the make steps which are currently used to collect the marionette tests. These are the following:

1. Calling print-manifest-dirs.py with the top-level manifest file
2. Creating a tar archive from all folders under topsrcdir as listed in the top-level manifest file
3. Extracting the tar archive to obj/dist/test-stage
4. test_archive.py picks those up to include in common.tests.zip

The improvement we want to have is:

1. Call test_archive.py which collects the tests and directly includes them in the target tests.zip file

With that change we save a full tar/untar cycle.
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
I got the print_manifests.py file integrated into test_archive.py. For testing I used the Marionette files as example and all is working perfectly. With it I also have removed the make file instructions in testing/testsuite-targets.mk.

I will massage the patch tomorrow and will upload a first WIP.
Summary: Improve test package archiver for collecting test files from different directories → Improve test package archiver for collecting files from directories referenced by a root manifest
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

Gregory, I would love to get some feedback for that patch before I start checking what kind of unit tests I could implement. Please let me know if it looks fine, or if things are wrongly implemented.
Attachment #8782006 - Flags: feedback?(gps)
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

https://reviewboard.mozilla.org/r/72298/#review70690

This looks mostly good!

::: python/mozbuild/mozbuild/action/test_archive.py:437
(Diff revision 2)
> +            '**/docs',
> +            '**/*.log',

The addition of these 2 scares me a little. I could see someone getting really confused as to why their directory or .log files aren't getting packaged in automation.

::: python/mozbuild/mozbuild/action/test_archive.py:465
(Diff revision 2)
> +    dirs.add(os.path.dirname(manifest))
> +
> +    # output the directories of all the other manifests
> +    test_manifest = TestManifest()
> +    test_manifest.read(os.path.join(topsrcdir, manifest))
> +    for i in test_manifest.get():
> +        d = os.path.dirname(i['manifest'])[len(topsrcdir) + 1:]
> +        dirs.add(d)

It seems you don't need the `topsrcdir` argument at all: you can just derive it from `os.path.dirname(manifest)`.

::: python/mozbuild/mozbuild/action/test_archive.py:474
(Diff revision 2)
> +    test_manifest.read(os.path.join(topsrcdir, manifest))
> +    for i in test_manifest.get():
> +        d = os.path.dirname(i['manifest'])[len(topsrcdir) + 1:]
> +        dirs.add(d)
> +
> +    dirs = ['{}/**'.format(p.replace('\\', '/')) for p in dirs]

We have the `mozpath` module that provides path manipulation functions that always use Unix style paths. You should use that throughout this function so you don't have to normalize.

The adding of the wildcard could also be added into the for loop above.
Attachment #8782006 - Flags: feedback?(gps) → feedback+
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

https://reviewboard.mozilla.org/r/72298/#review70690

> The addition of these 2 scares me a little. I could see someone getting really confused as to why their directory or .log files aren't getting packaged in automation.

Ok, I will get rid of both to keep parity with the current packaging.

> It seems you don't need the `topsrcdir` argument at all: you can just derive it from `os.path.dirname(manifest)`.

If you don't use topsrcdir here, the command "mach build package-tests" will fail if called from the obj dir. Given that it was supported before I didn't want to break it. Maybe I should add a comment here explaining it. Does it make sense to you?

> We have the `mozpath` module that provides path manipulation functions that always use Unix style paths. You should use that throughout this function so you don't have to normalize.
> 
> The adding of the wildcard could also be added into the for loop above.

Wasn't aware of that helpful module. My next commit will fix that.
I did some more work on this bug and was able to combine manifests with reftest manifests, so that we do no longer have a special case for the reftests. We can now define the test entry as any other one. After some cleanup and testing I will upload a new version of the patch.
Attachment #8782007 - Flags: review?(gps)
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

Gregory, due to a bug in mozreview I cannot request review from you for the first commit. Lets do it this way and I hope it will work.
Attachment #8782006 - Flags: review?(gps)
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

https://reviewboard.mozilla.org/r/72298/#review72010

::: python/mozbuild/mozbuild/action/test_archive.py:477
(Diff revision 3)
>      dirs = set()
> +
>      for p in manifests:
> +        p = os.path.join(topsrcdir, p)
> +
> +        if p.endswith('.ini'):

Maybe not that ideal by checking for the extension, but still better as having reftests handled separately. Or is there an existing way to check for a manifest type? I haven't found one, and don't want to necessarily touch both Manifest classes.
Gregory, will you have the time to review this patch or shall I ask someone else? Thanks.
Flags: needinfo?(gps)
Yes, I'll look at it.
Flags: needinfo?(gps)
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

https://reviewboard.mozilla.org/r/72298/#review72010

> Maybe not that ideal by checking for the extension, but still better as having reftests handled separately. Or is there an existing way to check for a manifest type? I haven't found one, and don't want to necessarily touch both Manifest classes.

There isn't a better way. Sniffing the file extension feels OK.
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

https://reviewboard.mozilla.org/r/72298/#review73314

This commit changes a lot of things. In the future, please consider splitting your changes into smaller parts to make review easier.
Attachment #8782006 - Flags: review?(gps) → review+
Comment on attachment 8782007 [details]
Bug 1283919 - Move packaging of Marionette from make to the test archiver

https://reviewboard.mozilla.org/r/72300/#review73318

This looks good.

Does this commit also remove the only users of the `MARIONETTE_*` moz.build variables? If so, there should be a follow-up to nuke those variables from moz.build. There are references in at least python/mozbuild/mozbuild/frontend/context.py and python/mozbuild/mozbuild/testing.py.
Attachment #8782007 - Flags: review?(gps) → review+
Comment on attachment 8782006 [details]
Bug 1283919 - Improve test package archiver for collecting files from directories referenced by a root manifest

https://reviewboard.mozilla.org/r/72298/#review73314

I know, but I wanted to make sure that I do not add or change code in a commit which is not working as expected. Splitting up all changes at the end is even harder for me locally. I will see what I can do the next time. Thanks.
Comment on attachment 8782007 [details]
Bug 1283919 - Move packaging of Marionette from make to the test archiver

https://reviewboard.mozilla.org/r/72300/#review73318

Oh! I totally missed, that there are references too. I will check this tomorrow but yes, nothing should remain in tree anymore.
Comment on attachment 8782007 [details]
Bug 1283919 - Move packaging of Marionette from make to the test archiver

https://reviewboard.mozilla.org/r/72300/#review73318

I got those definitions removed with the latest commit. So Marionette is completely gone from moz.build now.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e1f25f59298
Improve test package archiver for collecting files from directories referenced by a root manifest r=gps
https://hg.mozilla.org/integration/autoland/rev/a328778db08d
Move packaging of Marionette from make to the test archiver r=gps
https://hg.mozilla.org/mozilla-central/rev/7e1f25f59298
https://hg.mozilla.org/mozilla-central/rev/a328778db08d
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
As we figured out yesterday the removal of the moz.build entries of Marionette was a mistake which needs to be reverted. Ultimately this is the way how we want to collect tests.

We should get this backed out.
https://hg.mozilla.org/mozilla-central/rev/ab70808cd4b6
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla51 → ---
The faulty part of this patch is actually the second commit. It's about Marionette and actually does not really rely into this bug.

I will go ahead and strip of the second commit, and move it into a separate bug which will be for the Marionette changes. With that we can safely re-land the commit for the general test_archiver changes.
Attachment #8782007 - Attachment is obsolete: true
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3d1777fa2c34
Improve test package archiver for collecting files from directories referenced by a root manifest r=gps
https://hg.mozilla.org/mozilla-central/rev/3d1777fa2c34
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.