Skip to content
This repository has been archived by the owner on Jan 21, 2020. It is now read-only.

Disable Kiss Tracking tests on Travis #1046

Closed
wants to merge 12 commits into from

Conversation

fauvel
Copy link
Contributor

@fauvel fauvel commented Mar 4, 2014

Follow-up of #1033 and #1040

@fauvel fauvel mentioned this pull request Mar 4, 2014
@fauvel
Copy link
Contributor Author

fauvel commented Mar 4, 2014

@njam @kris-lab please review.

This is the result of a long series of trials and errors to find out which tests have been causing the Segfault happening on Travis, with PHP 5.4 and 5.5, after the code coverage analysis. I've used gdb and could see that the root cause was a PDO connection to some MySQL server, stored on some class member variable and freed automatically at the end of the script execution (Zend shutdown handler), that cannot send the "close" signal to the server any more because of a connection loss (broken pipe).

But it seems that neither our own PDO connection CM_Db_Client::$_pdo, nor the PDO connection used by the Amazon Web Services SDK, CachePDO::$pdo, are responsible for that. The problematic tests are using a Redis connection CM_Redis_Client::$_redis, stored (indirectly) on mock objects of type CM_KissTracking and CM_Action_Abstract.

Skipping the tests in CM_Action_AbstractTest helped, but for CM_KissTrackingTest, I even had to remove the file altogether to get a green build... It looks like PhpUnit's mere scanning of this test file was enough to cause a segmentation fault in the end!? If one of you guys understand what's happening... I don't.

@fauvel
Copy link
Contributor Author

fauvel commented Mar 4, 2014

@tomaszdurka could you please have a look into this, too? It looks like an issue with the Redis server and/or with the PHP extension used on Travis.

@fauvel
Copy link
Contributor Author

fauvel commented Mar 4, 2014

It's possible that upgrading phpredis on Travis could fix the problem. See this issue for an example.

@fauvel
Copy link
Contributor Author

fauvel commented Mar 4, 2014

I've tried it out in #1047, it doesn't help.

@fauvel
Copy link
Contributor Author

fauvel commented Mar 5, 2014

Now we get a green build for PHP 5.3, 5.4 and 5.5 (apart from the bug introduced by #1051, which will be fixed in #1053 and is not related to this issue).

@njam @tomaszdurka what about merging this PR now? The problem doesn't happen any more in PHP 5.5, probably thanks to an update on Travis. Maybe we can also remove this workaround for PHP 5.4 at some point, after Travis has updated its stack and fixed this issue. What do you think?

@njam
Copy link
Member

njam commented Mar 5, 2014

Briefly discussed with Tomasz.
Can you try emptying the whole CM_KissTrackingTest file?
If it works we can further isolate the problem?

Tomasz also noticed that in setUp we mock but don't implement _uploadCsv.

@fauvel
Copy link
Contributor Author

fauvel commented Mar 6, 2014

Can you try emptying the whole CM_KissTrackingTest file?

Now I'm deleting this file altogether in .travis.yml. Emptying it would work too. Actually it is sufficient not to derive the test class from PHPUnit_Framework_TestCase, so that it gets ignored by phpUnit. But what's the point in doing so?

@fauvel
Copy link
Contributor Author

fauvel commented Mar 6, 2014

Tomasz also noticed that in setUp we mock but don't implement _uploadCsv.

I've already tried to move the mocking to every single test method, and to place it after the call to markTestSkipped(), which throws an exception. It didn't help either.

@njam
Copy link
Member

njam commented Mar 6, 2014

I also did some testing (one, two, three), and give up as well..

Notes from removing certain tests (YES means the build suceeds):
3698505 adprovider-form YES
4f2163f config-file NO
7969242 config-dom NO
5901420 adprovider-asset YES
4676e97 adprovider-asset2 YES
214d539 cache-clockwork YES
5f3e3a9 cache-clockwork (with amazon) YES
448b52c cache (with amazon) YES (cfc missing)
382fca3 class (with amazon) YES (cfc missing)
3a8408f clockwork (with amazon) NO (cfc missing)
092acae - (with amazon) NO
0b16c71 class (with amazon) NO
af4318f adprovider-app NO
7e65676 app NO
102247a asset NO

I would suggest to remove the whole "Kiss"-functionality, wdyt?

@fauvel
Copy link
Contributor Author

fauvel commented Mar 7, 2014

We're not using kiss tracking any more, right? We've replaced it by our own action statistics? So we could probably drop it without hurting too much.

But... is it actually related to this issue? No idea... What your tests are showing could also be this simple thing: That we're avoiding the "root cause" – the connection timeout causing the broken pipe in the end – by removing enough tests. Or do you see some common pattern in the tests you've removed to get a green build?

If this is the case, we'll run into the same issue again as we add some more unit tests, independently of what we're actually testing. We would have to drop the whole AWS tests to get rid of that (if the AWS SDK PDO connection is really the problem).

So... I would rather suggest to wait and see. We could merge a simple workaround that make the tests pass for the moment, like removing CM_KissTrackingTest and CM_Action_AbstractTest in .travis.yml (no change in the code itself!), and see whether and when the tests are failing again. Maybe things becomes clearer then. Or maybe Travis will update PHP 5.4 and fix the problem (we're not having this issue in PHP 5.5), so that we could remove the workaround from .travis.yml at some point. In any case, it is definitely important to test cm in PHP 5.4, since we're using it in prod now, as far as I know. What do you think?

@njam
Copy link
Member

njam commented Mar 7, 2014

What I found out:

  • Removing Amazon API completely doesn't help
  • It doesn't seem timing related (disabling coverage and running a smaller set of tests still results in the problem).
  • It seems to be a complex combination of various things which trigger the error..

I'm not so fond of disabling tests (especially CM_Action_AbstractTest) on 5.4, because we use it in production. I think the chance that we're going to use Kiss-tracking again is very small, by that point they might have changed their API also (hopefully!).
Therefore I think removing the Kiss tests is one of the best options atm.
If this error pops up again, we need to look for causes / workarounds again anyways.
What do you think?

@fauvel
Copy link
Contributor Author

fauvel commented Mar 10, 2014

Well, if it works... but you haven't tested it for now, right? I had to remove CM_KissTrackingTest and to skip CM_Action_AbstractTest to avoid this Segfault, removing CM_KissTrackingTest only doesn't help. So maybe removing CM_KissTracking altogether (with all its usages) would be enough, but not sure... I'll give it a try.

Conflicts:
	tests/library/CM/Action/AbstractTest.php
	tests/travis/php-5.5.ini
@scrutinizer-notifier
Copy link

The inspection completed: No new issues

@fauvel
Copy link
Contributor Author

fauvel commented Mar 10, 2014

Segfault on PHP 5.4 & 5.5 :(

Although CM_Action_AbstractTest is still being skipped... It really doesn't make any sense to me.

@njam
Copy link
Member

njam commented Mar 13, 2014

Removing coverage doesn't help in any way? We might want to remove that anyway -> #1078

@njam
Copy link
Member

njam commented Mar 14, 2014

@fauvel fyi, scrutinizer and code coverage was removed in https://github.com/cargomedia/CM/pull/1078/files

@fauvel
Copy link
Contributor Author

fauvel commented Mar 17, 2014

Still segfaulting in PHP 5.4 & 5.5 with:

  • No code coverage
  • CM_Action_AbstractTest skipped
  • KissTrackingTest.php deleted

So the minimal fix I've found with code coverage doesn't work any more without...

@njam
Copy link
Member

njam commented Mar 17, 2014

omg :|

@tomaszdurka
Copy link
Contributor

@njam
Copy link
Member

njam commented Mar 19, 2014

@cm-jan @dlondero maybe you have some input/ideas?
We're basically stuck with the unit tests segfaulting on travis with php 5.4 and 5.5.
Removing some unit tests helps, but the patterns seem very random.
Some info on the segfault: #1033

@fauvel
Copy link
Contributor Author

fauvel commented May 2, 2014

Fixed by #1183?

@fauvel fauvel closed this May 2, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants