Skip to content

Commit

Permalink
Civi\API\ExternalBatch - Improve test for variables_order/$_ENV
Browse files Browse the repository at this point in the history 
The recent revision #9595 updated `ExternalBatch` to produce a more
informative error message.  However, in the test environment used by
`flexmailer`, there was exactly 1 value in `$_ENV` (even if
`variables_order` was misconfigured).

This should make the test a bit harder to trip-up by affirmatively checking
for the most common environment variable.

Review note: In the known universe, the only direct references to
`ExternalBatch` are in `CiviUnitTestCase`, so this shouldn't impact any
runtime logic.
  • Loading branch information
@totten
totten committed Mar 13, 2017
1 parent dad2a90 commit 40406b5
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion Civi/API/ExternalBatch.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ public function __construct($defaultParams = array()) {
$this->settingsPath = defined('CIVICRM_SETTINGS_PATH') ? CIVICRM_SETTINGS_PATH : NULL;
$this->defaultParams = $defaultParams;
$this->env = $_ENV;
if (empty($_ENV)) {
if (empty($_ENV['PATH'])) {
// FIXME: If we upgrade to newer Symfony\Process and use the newer
// inheritEnv feature, then this becomes unnecessary.
throw new \CRM_Core_Exception('ExternalBatch cannot detect environment: $_ENV is missing. (Tip: Set variables_order=EGPCS in php.ini.)');
Expand Down

3 comments on commit 40406b5

@JohnFF
Copy link
Contributor

@JohnFF JohnFF commented on 40406b5 Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is a requirement of the PHP config, should we inform the user if they don't have it set? We do that for a few other things, i.e. file perms.

Also, even after making that change, I still get the exception in api_v3_JobProcessMailingTest::testConcurrency ./scripts/phpunit --filter testConcurrency

  1. api_v3_JobProcessMailingTest::testConcurrency with data set webtests cleanup #6 (array(2, 10, 1, 5, 6), array(3, 1, 1), 20)
    Exception: CRM_Core_Exception: "ExternalBatch cannot detect environment: $_ENV is missing. (Tip: Set variables_order=EGPCS in php.ini.)"
    #0 /srv/www/exttest/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php(797): Civi\API\ExternalBatch->__construct((Array:2))
    CRM-11926 Improve error message on APi failure to include DB error message #1 /srv/www/exttest/sites/all/modules/civicrm/tests/phpunit/api/v3/JobProcessMailingTest.php(273): CiviUnitTestCase->createExternalAPI()
    CRM-11926 Improve error message on APi failure to include DB error message #2 internal function: api_v3_JobProcessMailingTest->testConcurrency((Array:5), (Array:3), 20)
    fixed the language and variable refering to svn #3 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/Framework/TestCase.php(909): ReflectionMethod->invokeArgs(Object(api_v3_JobProcessMailingTest), (Array:3))
    -- webtest cleanup (used openCiviPage function in place of open) #4 /srv/www/exttest/sites/all/modules/civicrm/tests/phpunit/CiviTest/CiviUnitTestCase.php(184): PHPUnit_Framework_TestCase->runTest()
    CRM-11926 add a better error handling for db errors in the api #5 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/Framework/TestCase.php(768): CiviUnitTestCase->runTest()
    webtests cleanup #6 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/Framework/TestResult.php(612): PHPUnit_Framework_TestCase->runBare()
    more webtest cleanup and code improvement  #7 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/Framework/TestCase.php(724): PHPUnit_Framework_TestResult->run(Object(api_v3_JobProcessMailingTest))
    update gitify help #8 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/Framework/TestSuite.php(747): PHPUnit_Framework_TestCase->run(Object(PHPUnit_Framework_TestResult))
    fix CRM-11995 #9 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/Framework/TestSuite.php(747): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult))
    Make helper fn for enabling components #10 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/TextUI/TestRunner.php(440): PHPUnit_Framework_TestSuite->run(Object(PHPUnit_Framework_TestResult))
    Crm 11534 #11 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/TextUI/Command.php(149): PHPUnit_TextUI_TestRunner->doRun(Object(PHPUnit_Framework_TestSuite), (Array:5))
    Fix for CRM-12003 #12 phar:///home/vagrant/buildkit/bin/phpunit4/phpunit/TextUI/Command.php(100): PHPUnit_TextUI_Command->run((Array:2), TRUE)
    Fix for CRM-12004 #13 /home/vagrant/buildkit/bin/phpunit4(545): PHPUnit_TextUI_Command::main()
    Fix for CRM-12005 #14 {main}

@totten
Copy link
Member Author

@totten totten commented on 40406b5 Apr 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only a requirement for ExternalBatch -- which is only used in that one test. Don't think it's worth the higher volume of questions/support/discussion that would come with making it a main requirement.

@JohnFF
Copy link
Contributor

@JohnFF JohnFF commented on 40406b5 Apr 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.

Please sign in to comment.