Issue 5997 - Avoid including qunit files in release builds

Issue 5997 - Avoid including qunit files in release builds

[kzar, 2017-11-07 15:25:40]

Patch Set 1

[kzar, 2017-11-07 15:28:36]

Patch Set 2 : Make use of base_extension_path variable

[kzar, 2017-11-07 16:08:12]

Patch Set 3 : Avoid bundling tests if testScripts option is missing

[tlucas, 2017-11-08 12:08:51]

Hey Dave!

I had a look at it - looks good so far, but something struck me. Are we
currently adding the qunit files 2 times? Please see below.

Best,
Tristan

https://codereview.adblockplus.org/29600577/diff/29600622/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29600577/diff/29600622/packagerChrome.py#n...
packagerChrome.py:393: add_devenv_requirements(files, metadata, params)
I wonder if this is/should be handled by webpack and/or if thisis also
responsible for the observed behavior - iirc, this was introduced by me parallel
to the webpack in #4720.

https://codereview.adblockplus.org/29600577/diff/29600622/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29600577/diff/29600622/tests/test_packager...
tests/test_packagerWebExt.py:246: )
You are adding these scripts but do not use it in any assertion. Please see
below.

https://codereview.adblockplus.org/29600577/diff/29600622/tests/test_packager...
tests/test_packagerWebExt.py:319: def assert_devenv_scripts(package, prefix,
devenv):
I guess here would be the correct place to assert for your introduced scripts.

[kzar, 2017-11-09 15:33:16]

Patch Set 4 : Add assertions for qunit bundle

https://codereview.adblockplus.org/29600577/diff/29600622/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29600577/diff/29600622/packagerChrome.py#n...
packagerChrome.py:393: add_devenv_requirements(files, metadata, params)
On 2017/11/08 12:08:51, tlucas wrote:
> I wonder if this is/should be handled by webpack and/or if thisis also
> responsible for the observed behavior - iirc, this was introduced by me
parallel
> to the webpack in #4720.

Well I agree it's kind of ugly how there are two separate checks for devenv. But
I don't think rendering the test page belongs in the code which creates the
webpack bundles, and I can't see how to add the metadata for the qunit bundle
outside of the create_bundles function. (I initially wanted to modify the
metadata from add_devenv_requirements to add the details about the qunit bundle
but it didn't look easily possible.)

https://codereview.adblockplus.org/29600577/diff/29600622/tests/test_packager...
File tests/test_packagerWebExt.py (right):

https://codereview.adblockplus.org/29600577/diff/29600622/tests/test_packager...
tests/test_packagerWebExt.py:246: )
On 2017/11/08 12:08:51, tlucas wrote:
> You are adding these scripts but do not use it in any assertion. Please see
> below.

Well they are used since the bundle is created, but true I should probably add
some assertions to ensure they are.

https://codereview.adblockplus.org/29600577/diff/29600622/tests/test_packager...
tests/test_packagerWebExt.py:319: def assert_devenv_scripts(package, prefix,
devenv):
On 2017/11/08 12:08:51, tlucas wrote:
> I guess here would be the correct place to assert for your introduced scripts.

Done.

[tlucas, 2017-11-09 16:29:50]

LGTM

https://codereview.adblockplus.org/29600577/diff/29600622/packagerChrome.py
File packagerChrome.py (right):

https://codereview.adblockplus.org/29600577/diff/29600622/packagerChrome.py#n...
packagerChrome.py:393: add_devenv_requirements(files, metadata, params)
On 2017/11/09 15:33:16, kzar wrote:
> On 2017/11/08 12:08:51, tlucas wrote:
> > I wonder if this is/should be handled by webpack and/or if thisis also
> > responsible for the observed behavior - iirc, this was introduced by me
> parallel
> > to the webpack in #4720.
> 
> Well I agree it's kind of ugly how there are two separate checks for devenv.
But
> I don't think rendering the test page belongs in the code which creates the
> webpack bundles, and I can't see how to add the metadata for the qunit bundle
> outside of the create_bundles function. (I initially wanted to modify the
> metadata from add_devenv_requirements to add the details about the qunit
bundle
> but it didn't look easily possible.)

Acknowledged.