Page MenuHomePhabricator

Running prod daemons fails on test files
Closed, ResolvedPublic

Description

We upgraded phabricator, arcanist, and libphutil last Friday. After adding our extensions, bin/phd start failed with

PHP Fatal error:  Class DropboxAddReviewersEnginePatched may not inherit from final class (DropboxAddReviewersEngine) in ..../phabricator/src/extensions/phabricator-extensions/contacts/herald/__tests__/DropboxAddReviewersEngineTest.php on line 15

I'm confused why the phabricator daemons are loading the testfiles -- particularly as I can't get them to run with arc unit, when they're supposed to. (strace shows they're not even looking at the test subdirectories).

What do you expect to be the minimal configuration is to run PhutilUnitTestEngine tests (aka tests directories)?

Event Timeline

angie updated the task description. (Show Details)
angie added a project: Restricted Project.
angie added subscribers: jhurwitz, angie, alexmv.

We always load everything in src/extensions/ when starting up, which is why this is loading.

For normal libraries (those mapped with arc liberate), we have a library map (in src/__phutil_library_map__.php) which lists the entire contents of the library. We use this map to load classes on-demand. For loose files in src/extensions/, we don't have that map, so we can't do an on-demand load. Instead, we just load everything immediately. The general assumption is that this directory will contain a handful of small files, and that larger pieces of functionality will be split out as real libraries.

I'm not 100% sure how the test engine and stuff in src/extensions/ interact, but I would expect us to never run tests in src/extensions/. It's possible that we do sort of by accident, but anything which is complex enough to merit unit tests should probably be moved to a real library.

The Adding New Classes document in the documentation has some details about this, and more detailed discussion about extensions vs libraries.

I'd guess that whatever contacts is is likely big/complex enough to deserve its own library. If you split it, you'll define the test engine in a .arcunit file in that library, run tests by running arc unit in contacts/, etc.

We don't have an example of how to build a simple skeleton app right now since we don't formally support third-party application development yet (see T5447) but I think the document above is reasonably thorough. There are also some existing third-party projects like WMF's Sprint application which might be helpful or maybe some examples on Community Resources, although that stuff generally does not follow best practices and may be somewhat misleading (but there are largely no documented best practices since there's no formal upstream support, so this isn't anyone's fault).

Let me know if that helps or you still have questions?

@alexmv feel free to follow up, otherwise I will close this after the next upgrade

If you've forked, you can also just move stuff into src/ourcustomstuff/ and use arc liberate, arc unit, etc., at top level. This may have an overall lower maintenance cost than splitting a library out given that you're already bearing the cost of maintaining a fork. Only src/extensions/ has magic behavior.

epriestley claimed this task.

Assuming this is in good shape now.

angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Dec 11 2015, 8:22 PM
angie moved this task from Restricted Project Column to Restricted Project Column on the Restricted Project board.Apr 21 2016, 10:22 PM