Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mitigate flaky test failure about "CRM_Utils_Check_Component_Env->checkVersion()" #17038

Merged
merged 1 commit into from
Apr 9, 2020

Conversation

totten
Copy link
Member

@totten totten commented Apr 9, 2020

Overview

The function CRM_Utils_Check_Component_Env->checkVersion() relies on an external data feed. If there's an error fetching feed, then we wind up with a PHP warning. This bubbles up to false-negatives in the test suite. This problem should be tracked in a different way.

Before

This warning can manifest on random pages:

Screen Shot 2020-04-08 at 9 33 47 PM

It can also manifest as random test-failures. I recently saw these false-negatives:

CRM_Contact_Page_View_UserDashBoardTest.testDashboardContentContributionsWithInvoicingEnabled
CRM_Contact_Page_View_UserDashBoardTest.testDashboardContentContributions
CRM_Core_Page_HookTest.testFormsCallBuildFormOnce
CRM_Core_Page_HookTest.testPagesCallPageRunOnce

After

The warning is suppressed.

Technical Details

To test this, I used the following to reproduce the problem and observe the mitigation:

diff --git a/CRM/Utils/Check.php b/CRM/Utils/Check.php
index e8eb744ab2..097f36052c 100644
--- a/CRM/Utils/Check.php
+++ b/CRM/Utils/Check.php
@@ -66,7 +66,7 @@ class CRM_Utils_Check {
   public function showPeriodicAlerts() {
     if (CRM_Core_Permission::check('administer CiviCRM')) {
       $session = CRM_Core_Session::singleton();
-      if ($session->timer('check_' . __CLASS__, self::CHECK_TIMER)) {
+      if (TRUE || $session->timer('check_' . __CLASS__, self::CHECK_TIMER)) {

         // Best attempt at re-securing folders
         $config = CRM_Core_Config::singleton();
diff --git a/CRM/Utils/VersionCheck.php b/CRM/Utils/VersionCheck.php
index 0b3b9fa460..bc69a65660 100644
--- a/CRM/Utils/VersionCheck.php
+++ b/CRM/Utils/VersionCheck.php
@@ -114,6 +114,7 @@ class CRM_Utils_VersionCheck {
    *     Ex: 'info', 'notice', 'warning', 'critical'.
    */
   public function getVersionMessages() {
+    return NULL;
     return $this->isInfoAvailable ? $this->versionInfo : NULL;
   }

Comments

I'm not usually a fan of suppressing warnings. It is legit to have some QA signals if latest.civicrm.org is unavailable. However, its better to use uptime monitoring for that web-service. It shouldn't manifest in random unit-tests or PHP warnings on random pages.

…ckVersion()"

This function relies on an external data feed. If it fails to fetch the feed, then we
wind up with a PHP warning:

```
Invalid argument supplied for foreach() in CRM_Utils_Check_Component_Env->checkVersion()
(line 475 of /home/me/bknix/build/dmaster/web/sites/all/modules/civicrm/CRM/Utils/Check/Component/Env.php).
```

In certain unit-tests, this warning becomes a false-negative test-failure.
I saw these tests failing recently:

```
CRM_Contact_Page_View_UserDashBoardTest.testDashboardContentContributionsWithInvoicingEnabled
CRM_Contact_Page_View_UserDashBoardTest.testDashboardContentContributions
CRM_Core_Page_HookTest.testFormsCallBuildFormOnce
CRM_Core_Page_HookTest.testPagesCallPageRunOnce
```

Note that it is legit to have some QA signals if the web-service fials, but
that's more of a monitoring issue for the web-service.  It shouldn't
manifest in random unit-tests or random page-views.
@civibot
Copy link

civibot bot commented Apr 9, 2020

(Standard links)

@civibot civibot bot added the master label Apr 9, 2020
@colemanw colemanw merged commit 8cdf551 into civicrm:master Apr 9, 2020
@totten totten deleted the master-vermsg branch April 9, 2020 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants