veda-consulting-company / uk.co.vedaconsulting.mailchimp

Other
22 stars 43 forks source link

Do the tests pass out of the box? #233

Closed xurizaemon closed 6 years ago

xurizaemon commented 7 years ago

I've set the module up and configured as I understand it ought to be, but I see quite a few test failures.

I'm happy to work on improving this but would appreciate the developers confirming whether they think the tests pass currently. (Would be useful even if you can just run the tests and say "we see similar errors" or "everything passes".)

Here's what I get right now from phpunit sites/default/files/civicrm/ext/uk.co.vedaconsulting.mailchimp/tests/integration- https://gist.github.com/xurizaemon/492945f14716c83cf4d783877569c284

Hit me up over on chat.civicrm.org if you wish - keen to improve this!

xurizaemon commented 7 years ago

Running tests on various branches - both on a "stock" current Drupal 7 + CiviCRM 4.7.11 via buildkit master.

artfulrobot commented 7 years ago

Those two branches are identical*, it does not make sense that veda's has 300 tests.

*identical: well 6 days ago artfulrobot/master was one commit behind veda's, but that was just veda's merge and so the code was still identical.

I've just re-tested on 4.7.8 (installed from standard download, not by buildkit) on Drupal 7, using 922edef from artfulrobot/master.

The integration tests with phpunit 5.2.12 ran fine:

phpunit.phar sites/default/filphpunit.phar sites/default/files/civicrm/ext/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrationMockTest.php
PHPUnit 5.2.12 by Sebastian Bergmann and contributors.

....................................                              36 / 36 (100%)
xurizaemon commented 7 years ago

Hmm. I'm definitely seeing failures. Would you mind comparing some versions so I can see what might be differing? Composer run from within the extension directory.

$ phpunit --version
PHPUnit 5.5.4 by Sebastian Bergmann and contributors.

$ php -v
PHP 5.6.21 (cli) (built: Jul 20 2016 12:45:50)
Copyright (c) 1997-2016 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2016 Zend Technologies

$ composer show
doctrine/instantiator             1.0.5  A small, lightweight utility to instantiate objects in PHP without invoking their constru...
phpdocumentor/reflection-common   1.0    Common reflection classes used by phpdocumentor to reflect the code structure
phpdocumentor/reflection-docblock 3.1.0  With this component, a library can provide support for annotations via DocBlocks or other...
phpdocumentor/type-resolver       0.2
phpspec/prophecy                  v1.6.1 Highly opinionated mocking framework for PHP 5.3+
sebastian/comparator              1.2.0  Provides the functionality to compare PHP values for equality
sebastian/diff                    1.4.1  Diff implementation
sebastian/exporter                1.2.2  Provides the functionality to export PHP variables for visualization
sebastian/recursion-context       1.0.2  Provides functionality to recursively process PHP variables
webmozart/assert                  1.1.0  Assertions to validate method input/output with nice error messages.
artfulrobot commented 7 years ago

I have now tried with a buildkit install and confirm I get errors on integration mock test:

PHPUnit 5.5.5 by Sebastian Bergmann and contributors.

.....EE.....................F........                             37 / 37 (100%)

Time: 30.67 seconds, Memory: 72.75MB

There were 2 errors:

1) MailchimpApiIntegrationMockTest::testPostHookForMembershipListChanges
CiviCRM_API3_Exception: Method call:
  - patch("/lists/dummylistid/members/55c1568355881d9b1b41c6"..., ["status" => "unsubscribed"])
on Double\CRM_Mailchimp_Api3\P3 was not expected, expected calls were:
  - put(exact("/lists/dummylistid/members/55c1568355881d9b1b41c6"...), callback())

/home/rich/civicrm-buildkit/build/mytestbuild/sites/all/modules/civicrm/api/api.php:45
/home/rich/civicrm-buildkit/build/mytestbuild/sites/default/files/civicrm/ext/uk.co.vedaconsulting.mailchimp/tests/integration/MailchimpApiIntegrationMockTest.php:280

2) MailchimpApiIntegrationMockTest::testPostHookForInterestGroupChanges
CiviCRM_API3_Exception: Method call:
  - patch("/lists/dummylistid/members/55c1568355881d9b1b41c6"..., ["status" => "unsubscribed"])
on Double\CRM_Mailchimp_Api3\P5 was not expected, expected calls were:
  - put(exact("/lists/dummylistid/members/55c1568355881d9b1b41c6"...), callback())

will look into it

xurizaemon commented 7 years ago

Great, yep that looks like what I was seeing - patches instead of puts.

artfulrobot commented 7 years ago

Ah, found the problem! CiviCRM's API changed its response to Contact:getsingle with return=group: it used to return a comma separated list of group titles, it now returns group ids!

Grrr. In order to fix this I'll have to have a way to know which exact version of CiviCRM changed this behaviour, and then check for that in the code and handle it two different ways depending on that.

xurizaemon commented 7 years ago

You might like to see #240 on that front ...

artfulrobot commented 7 years ago

Yeah, so what do you think should be done? Can you find out a way to figure out what CiviCRM's API is going to return for its groups value?

xurizaemon commented 7 years ago

I'm OK with treating them as IDs if the returned "groups" value has no letters or spaces in it (per comment just posted over on #240).

artfulrobot commented 7 years ago

OK, I get the integration mock tests working now. I pulled your master branch, cherry-picked your group titles code, then added a further commit to fix it so that it only returns the group ids passed in $group_details (which is important).

https://github.com/artfulrobot/uk.co.vedaconsulting.mailchimp/commit/a7e8543b8916a9020c2b801c0e2b1c08a7bc27c1 (fixes #240)

I get all 37 mock tests passing now. I've got two failures in MailchimpApiIntegrationTest::testPullContactWithOtherEmailDiff and MailchimpApiIntegrationTest::testPullIgnoresDuplicates though that I've not yet looked into

artfulrobot commented 7 years ago

Ok so now the only thing stopping the tests running AFAICS is Mailchimp's rate limiting.

xurizaemon commented 7 years ago

Great. Btw, as noted in #240 and CRM-19426, this change arrived - apparently unexpectedly and without fanfare - in CiviCRM 4.7.11.

artfulrobot commented 7 years ago

Well it's definitely an improvement, but wish API changes had a little more fanfare! I've added a note in the code to remove the old method once everyone's beyond that version No.

Many thanks.

veda-consulting commented 6 years ago

Closing this - tests should be passing