Apparently --enable-tests changes the way how we compile files. https://dxr.mozilla.org/mozilla-central/search?q=ENABLE_TESTS+path%3A.cpp&redirect=false&case=false give a lot of entries, where we use #ifdefs to compile different code. It could be similar for other files types (Makefiles, headers, etc). One of the ideas of build promotion is to ship the same code that we test, but this looks like a show stopper here...

looking at some of this: https://dxr.mozilla.org/mozilla-central/source/dom/media/ADTSDemuxer.cpp#196 * support for previous frame- lets dig into why this is here and why it is needed. It was added in bug 1169212. Dan, can you help us figure out why we have prevFrame behind ENABLE_TESTS? Is there a reason not to put this in production builds? If we shouldn't put this in a production build, can we solve this another way?

this is also the case for https://dxr.mozilla.org/mozilla-central/source/dom/media/MP3Demuxer.cpp

in bug 961049, we added enable_tests support to ActorsParent.cpp: https://dxr.mozilla.org/mozilla-central/source/dom/quota/ActorsParent.cpp this seems to support the disk space data while running xpcshell tests. :janv, is this something we can put into a production build? If not, is there a way we could test this functionality without the enable_tests?

in bug 1013571, support for enable_tests in RuntimeService.cpp was added: https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp these changes look to add test methods, I am not sure if this is something reasonable to add into a production build. :bkelly, can you add more info here and help us determine if we can ship a release build with this test code or if we can change the code or tests to make this work?

changes in: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp come from 2 bugs, bug 844288 and bug 767231. These are both the same area of code and adding a hook to run gtests. :BenWa, if we are looking to ship release builds with --enable-tests defined, are these gtest changes in nsAppRunner.cpp reasonable to ship? Are there other ways to solve this?

(In reply to Joel Maher (:jmaher) from comment #4) > in bug 1013571, support for enable_tests in RuntimeService.cpp was added: > https://dxr.mozilla.org/mozilla-central/source/dom/workers/RuntimeService.cpp > > these changes look to add test methods, I am not sure if this is something > reasonable to add into a production build. This was modeled off the existing ENABLE_TESTS in ContentParent.cpp for testing PBackground. That was introduced by :bent. In general I think exposing test IPC messages is not a good idea for release. An attacker could craft messages to parent using this actor. Since its test code it does not get the scrutiny other code gets. In theory we have enough real features using PBackground now, though, that maybe we could remove the PBackground test actor completely. Kyle, what do you think?

We use it this way: const bool QuotaManager::kRunningXPCShellTests = #ifdef ENABLE_TESTS !!PR_GetEnv("XPCSHELL_TEST_PROFILE_DIR") #else false #endif ; So quota/indexedDB code behaves differently if kRunningXPCShellTests is true which we definitely don't want for production. ENABLE_TESTS itself doesn't trigger different behavior, XPCSHELL_TEST_PROFILE_DIR must be set too. I think we can just remove the ENABLE_TESTS check in this case. It will cost us one more PR_GetEnv() call in production builds.

(In reply to Ben Kelly [:bkelly] from comment #6) > In theory we have enough real features using PBackground now, though, that > maybe we could remove the PBackground test actor completely. Kyle, what do > you think? I think :mrbkap reviewed that PBackground code, he might have an opinion on that too.

in bug 956218 we added ENABLE_TESTS support in ContentParent.cpp: https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp :bent, can you weigh in on how we could (if possible) resolve the enable_tests such that we could ship a release build from a build we actually test. It sounds like we don't want this code in a release build- maybe an addon or possibly we don't need these tests/code anymore?

:janv, regarding XPCSHELL_TEST_PROFILE_DIR, do you want to use this current bug, or would you like me to file a new bug. Also would you like me to create the patch, or is this something you want to do?

(In reply to Joel Maher (:jmaher) from comment #10) > :janv, regarding XPCSHELL_TEST_PROFILE_DIR, do you want to use this current > bug, or would you like me to file a new bug. Also would you like me to > create the patch, or is this something you want to do? It should be pretty simple patch, feel free to do it if you have time, I'll review.. A new bug is not needed.

I believe the last big of ENABLE_TESTS trickery is related to bug 1166207 and files: https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageLink.cpp https://dxr.mozilla.org/mozilla-central/source/mozglue/build/Nuwa.cpp :cyu, this is a lot of code executed, it doesn't look like something we want in release builds. I do question if we are actually running tests that use this now that b2g is tier3. Can you help us figure out if we can ship with this, we can solve this problem another way, or if we can remove this code?

11:22 AM <Callek> jmaher: using path:.h and removing path: entirely on the ENABLE_TESTS bits is also useful, I suspect there is at least one or two things lurking in the makefiles as well (though makefiles should be generally clear)

If you're asking about the conditional #include then yes it's fine. In fact I think you can remove it entirely because we forward declare what we need here anyways: https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp#267

I'm not opposed to removing the PBackground testing infrastructure now that it has real in-tree consumers.

I agree with Kyle.

(In reply to Joel Maher (:jmaher) from comment #12) > I believe the last big of ENABLE_TESTS trickery is related to bug 1166207 > and files: > https://dxr.mozilla.org/mozilla-central/source/ipc/glue/MessageLink.cpp > https://dxr.mozilla.org/mozilla-central/source/mozglue/build/Nuwa.cpp > > :cyu, this is a lot of code executed, it doesn't look like something we want > in release builds. I do question if we are actually running tests that use > this now that b2g is tier3. Can you help us figure out if we can ship with > this, we can solve this problem another way, or if we can remove this code? b2g tests are still run on try and b2g-inbound. Connected device and TV are consumers of this code. I think we can make it #ifdef MOZ_NUWA_PROCESS instead.

Joel, I cribbed off MP3Demuxer when creating ADTSDemuxer. MP3Demuxer had ENABLED_TESTS and I just copied it. The code is only an aid for debugging. I think it would be safe to remove the ENABLED_TESTS parts entirely from ADTSDemuxer and not effect functionality.

/me defers to mrbkap & khuey

thanks everyone for the info- I am working on creating bugs and patches to remove ifdef statements as discussed :)

(In reply to Joel Maher (:jmaher) from comment #20) > thanks everyone for the info- I am working on creating bugs and patches to > remove ifdef statements as discussed :) You are the best! :) TYVM!

according to dxr we have 0 results: https://dxr.mozilla.org/mozilla-central/search?q=ENABLE_TESTS+path%3A.cpp&redirect=false&case=false looking at all other instances of ENABLE_TESTS (mostly makefile/moz.build), I see one thing to look into: https://dxr.mozilla.org/mozilla-central/search?q=DOM_STORAGE_TESTS&redirect=false&case=false and we have predictor_tests defined but nothing using it, :mcmanus, can that define be removed: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/moz.build#312 :honza, can you comment on if we ship a release build with DOM_STORAGE_TEST=1 is that a bad thing? For reference here is the list via dxr: https://dxr.mozilla.org/mozilla-central/search?q=DOM_STORAGE_TESTS&redirect=false&case=false

nick is the right person to ask

(In reply to Joel Maher (:jmaher) from comment #22) > :honza, can you comment on if we ship a release build with > DOM_STORAGE_TEST=1 is that a bad thing? For reference here is the list via > dxr: > https://dxr.mozilla.org/mozilla-central/ > search?q=DOM_STORAGE_TESTS&redirect=false&case=false Jan Bambas should know more about this I guess. Honza

(In reply to Joel Maher (:jmaher) from comment #22) > according to dxr we have 0 results: > https://dxr.mozilla.org/mozilla-central/search?q=ENABLE_TESTS+path%3A. > cpp&redirect=false&case=false > > looking at all other instances of ENABLE_TESTS (mostly makefile/moz.build), > I see one thing to look into: > https://dxr.mozilla.org/mozilla-central/ > search?q=DOM_STORAGE_TESTS&redirect=false&case=false > > and we have predictor_tests defined but nothing using it, :mcmanus, can that > define be removed: > https://dxr.mozilla.org/mozilla-central/source/netwerk/base/moz.build#312 > > > :honza, can you comment on if we ship a release build with > DOM_STORAGE_TEST=1 We do, I think. Whenever tests are enabled (ENABLE_TESTS=1) at compilation time (which is IMO always) we do. > is that a bad thing? No, none of this is dangerous, exploitable or exposed to content. These test snippets are never used, just by some of the tests that run on any build we release, I presume. > For reference here is the list via > dxr: > https://dxr.mozilla.org/mozilla-central/ > search?q=DOM_STORAGE_TESTS&redirect=false&case=false

Thanks Honza! I will not worry about DOM_STORAGE_TESTS ifdef and move on. The only other code work I see is potentially removing the ifdef in necko if it isn't used- I will wait on :hurley for that, but that shouldn't block this bug. :rail, as you filed this bug on code specific issues, can we close this?

There is one thing left that I wanted to look at today - packaging. I'll close as soon as we are done. Thanks a lot!

(In reply to Joel Maher (:jmaher) from comment #22) > and we have predictor_tests defined but nothing using it, :mcmanus, can that > define be removed: > https://dxr.mozilla.org/mozilla-central/source/netwerk/base/moz.build#312 Yep, that can go.

Joel, We want to switch to release promotion starting 46.0b1 (go to build in ~8 days). Do you think it's worth uplifting the patches to aurora next week so they migrate to beta?

yeah, I think that sounds reasonable.

requested all the uplift requests.

all patches are on aurora!