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

MongoDd: fixed expiration support and removed duplicated tests #25

Closed
wants to merge 2 commits into from
Closed

MongoDd: fixed expiration support and removed duplicated tests #25

wants to merge 2 commits into from

Conversation

marc-mabe
Copy link
Member

with an edge case in MongoDb::internalGetItem()

@Maks3w
Copy link
Member

Maks3w commented Oct 25, 2015

Any possibility of add a test?

@marc-mabe
Copy link
Member Author

I have seen a test failure in the develop branch. I'll check if the test fails also in master else I add it.

@marc-mabe
Copy link
Member Author

@Maks3w I found out that the test MongoDbTest::testCachedItemsExpire only failed sometimes (see https://travis-ci.org/marc-mabe/zend-cache/jobs/87353080) and noticed that the expiration feature wasn't correctly defined in capabilities. It was minTtl=0 which means this adapter doesn't support expiration of items. So I fixed the capabilities and also removed all capabilities that are not needed to define because of default.

I also noticed that MongoDB natively supports expiration of items by an index but this index wasn't defined -> so I added this index.

-> Because this index will only be defined if the collection wasn't pushed into the resource manager I leave the logic in the adapter intact so it will also work with the index.

At this point I also removed the duplicated tests:

  • MongoDbTest::testCachedItemsExpire() already done by CommonAdapterTest::testGetItemReturnsNullOnExpiredItem()
  • MongoDbTest::testFlush() already done by CommonAdapterTest::testFlush()

@marc-mabe marc-mabe changed the title fixed 'Undefined variable: key' MongoDd: fixed expiration support and removed duplicated tests Oct 31, 2015
@marc-mabe marc-mabe added the bug label Oct 31, 2015
@Maks3w
Copy link
Member

Maks3w commented Nov 1, 2015

I've doubts about what expire strategy is tested then. I think we need two test cases for test when MongDb is who expire or when the defined logic

@marc-mabe
Copy link
Member Author

mh I don't know why but if I remove the adapter logic to handle expired items all TTL tests fails even with this index set : https://travis-ci.org/marc-mabe/zend-cache/jobs/88634000

So the logic for handling of expired items needs to be in the adapter even with this index and the logic gets covered with the current tests.

@Maks3w
Copy link
Member

Maks3w commented Nov 1, 2015

Ok. Seems the expires index is a new feature and its not related with the original issue.

If it is correct could you split the PR and propose the index feature as enhancement?

@marc-mabe
Copy link
Member Author

@Maks3w I removed the line enabling the special index.
(I'll create a new PR for it later)

@marc-mabe
Copy link
Member Author

mh coverage for the Storage/Adapter/MongoDb.php increased from 82.0 to 82.29 but APC related coverage get reported as 0 -> I have no idea why as I didn't change something related to it.

@marc-mabe marc-mabe added the Test label Nov 1, 2015
@marc-mabe marc-mabe added this to the 2.5.4 milestone Nov 4, 2015
@marc-mabe marc-mabe removed the Test label Nov 7, 2015
@marc-mabe
Copy link
Member Author

merged with 5cda324 and d2cf8dd

@marc-mabe marc-mabe closed this Nov 29, 2015
@marc-mabe marc-mabe modified the milestones: 2.6.0, 2.5.4 Feb 12, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants