1735814 - Stop using preprocessor in manifest.jsm

Status: RESOLVED FIXED

(Testing :: Reftest, enhancement)

Description

[Marco Castelluccio [:marco]]

The preprocessor is only used in few places (e.g. https://searchfox.org/mozilla-central/rev/46ff2252568db36e811109fa4026c8e3c12e9ee1/layout/tools/reftest/manifest.jsm#552), we should stop using it.

We should be able to move to use AppConstants (like https://searchfox.org/mozilla-central/rev/46ff2252568db36e811109fa4026c8e3c12e9ee1/toolkit/modules/UpdateUtils.jsm#1058) instead of all of those preprocessor usages.

Once we've changed tools/reftest/manifest.jsm to use AppConstants instead of the preprocessor, we can remove the "*" before manifest.jsm in layout/tools/reftest/jar.mn, which means the preprocessor won't be used anymore for this file.

Comment 1

[Shazib Summar [:ssummar]]

Are there any tests I can run locally to confirm I didnt break anything?

Comment 2

[Marco Castelluccio [:marco]]

Yes, same as bug 1475503, something like ./mach crashtest accessible/tests/crashtests/471493.xul.

Comment 3

[Shazib Summar [:ssummar]]

Attachment: Bug 1735814 - Stop using preprocessor in manifest.jsm r=marco

Comment 4

[Shazib Summar [:ssummar]]

Added a patch. Let me know if any revisions are needed. Also, the test ./mach crashtest accessible/tests/crashtests/471493.xul is giving me errors.

REFTEST SUITE-START | Running 0 tests
REFTEST ERROR | EXCEPTION: No tests to run
REFTEST INFO | Result summary:
REFTEST INFO | Successful: 0 (0 pass, 0 load only)
REFTEST INFO | Unexpected: 1 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 failed load, 1 exception)
REFTEST INFO | Known problems: 0 (0 known fail, 0 known asserts, 0 random, 0 skipped, 0 slow)
REFTEST SUITE-END | Shutdown
REFTEST INFO | Slowest test took 0ms (undefined)

Full error log is here. However, both ./mach build and ./mach build layout/tools/reftest completed without errors so I pushed the patch.

Also, can you please help me understand why we are removing preprocessors from this file? In bug 1475503 you mentioned something about rebuilding being painful. Could you please elaborate on that part? Thanks.

Comment 5

[Marco Castelluccio [:marco]]

(In reply to Shazib Summar from comment #4)

Added a patch. Let me know if any revisions are needed. Also, the test ./mach crashtest accessible/tests/crashtests/471493.xul is giving me errors.

REFTEST SUITE-START | Running 0 tests
REFTEST ERROR | EXCEPTION: No tests to run
REFTEST INFO | Result summary:
REFTEST INFO | Successful: 0 (0 pass, 0 load only)
REFTEST INFO | Unexpected: 1 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 failed load, 1 exception)
REFTEST INFO | Known problems: 0 (0 known fail, 0 known asserts, 0 random, 0 skipped, 0 slow)
REFTEST SUITE-END | Shutdown
REFTEST INFO | Slowest test took 0ms (undefined)

Full error log is here. However, both ./mach build and ./mach build layout/tools/reftest completed without errors so I pushed the patch.

Try with ./mach crashtest accessible/tests/crashtests.

Also, can you please help me understand why we are removing preprocessors from this file? In bug 1475503 you mentioned something about rebuilding being painful. Could you please elaborate on that part? Thanks.

The JS files containing preprocessor directives are not standard JS files and so they have a few problems:

• you need to "build" them to convert them to normal JS, which is what I mentioned in bug 1475503;
• being non-standard, it is harder to analyze them with the usual JS tools;
• we have code coverage data for the built JS, and we have to map it back to the original "JS with preprocessor" file (since the lines from the original and the built do not match, as the preprocessor directives are removed), which is suboptimal.

Comment 6

[Shazib Summar [:ssummar]]

(In reply to Marco Castelluccio [:marco] from comment #5)

Try with ./mach crashtest accessible/tests/crashtests.

Yeah, that works. Thanks.

REFTEST INFO | Result summary:
REFTEST INFO | Successful: 18 (0 pass, 18 load only)
REFTEST INFO | Unexpected: 0 (0 unexpected fail, 0 unexpected pass, 0 unexpected asserts, 0 failed load, 0 exception)
REFTEST INFO | Known problems: 0 (0 known fail, 0 known asserts, 0 random, 0 skipped, 0 slow)
REFTEST SUITE-END | Shutdown

Also, how's the patch?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bfe8a37f487
Stop using preprocessor in manifest.jsm r=marco DONTBUILD

Comment 8

[Natalia Csoregi [:nataliaCs]]

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