Long running branches, static variables, and other rants

[totten]

So I've been working to resolve a test-failure in the "master-civimail-abtest" branch, and it's been rather... difficult (8hr? 12hr? 20yr? I can't remember anymore.). The final patch:

https://github.com/totten/civicrm-core/commit/158951c84e9298e201b2881a33259066604301ff

A few suggestions on things which could be done better to avoid this situation...

Use a PR to test long-running branches

When you make a branch that's going to last for a while (weeks or months), it's a good idea to open a PR and periodically send your code for testing -- even if you don't think the code is ready to be merged. (Just make sure to put a label or comment on the PR like "DO NOT MERGE".) Since PR's are automatically tested, your branch will be automatically tested. The CI will usually test something that you haven't tested locally. This can help catch such idiosyncrasies sooner rather than latter.

Static variables are evil.

Many developers before me have said it, but I'll repeat -- don't drop static variables into random places. These make it extremely hard to unit-test by creating unseen side-effects and mysterious interactions between test-cases. If you find the temptation to make one, look for an alternative. If you can't think of a good alternative, then reach out to another developer over forum/IRC/email/Github/JIRA/smoke-signals/anything to see if someone else has a suggestion.

(Note: These comments are directed mostly at core code.)

[Coleman Watts]

@totten I'm trying to understand better the best-practices for static variables. We seem to have a lot of them. Many seem to be implemented for the sake of performance (although we have so many overlapping cache layers I'm not sure how real those benefits are).
What's your stance, on, say, this one: https://github.com/civicrm/civicrm-core/blob/master/CRM/Core/SelectValues.php#L72

[totten]

Agree that the situation has grown a bit... complex, and I don't have a blanket statement, "We should always do X and should change everything right now." However, it may be useful to define what's good/bad and sort some examples from worst-practice to best-practice.

Criteria

• Obvious - If you're writing a test, would you realize that you need to clean this up?
• Resettable - If you know it exists, is it possible to cleanup/reset it?
• Dynamism - Does the data in the variable change in a substantive way during normal execution? If the data is used dynamically, then it requires more thought to understand how it behaves over time; if it is used statically (with unchanging data -- like a cache), then it's simpler to understand, and there's a large % of test-cases where it doesn't matter if you forget about it.

Examples of static-y things (Roughly ordered worst to best)

• The old $mailsProcessed example (which was removed by 158951c8 above) is the worst. It's not obvious, not resettable, and not genuinely static.
• The new $mailsProcessed example (added by 158951c8 -- using a static class var) is resettable (but not obvious and not genuinely static).
• The CRM_Core_SelectValues example is neither resettable nor obvious, but at least the data is genuinely static (in most use-cases).
• CRM_Extension_System::singleton($fresh) and CRM_Utils_Hook::singleton($fresh) are a little more obvious (because they apply to broad subsystems) --  but not obvious on the whole. They are resettable and static (in most use-cases).
• Civi\Core\Container::singleton() is obvious (there's only one container in the entire app) as well as resettable and static (in most use-cases).

Generally, the best case is to put "static-y" things into the container -- and to reset the entire container between tests.