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

Fix broken CacheResolver tests (#650) #655

Merged
merged 1 commit into from
Nov 1, 2015

Conversation

kamazee
Copy link
Contributor

@kamazee kamazee commented Oct 30, 2015

Fixes #650

Since September'2015 Doctrine doesn't always save namespace version in
the cache itself

Relying on such kind of implementation detail in a test that is supposed to check if several keys are saved in cache is kind of pointless, so it makes sense to ignore the cache key that holds a namespace version -- that's exactly what's implemented in the PR.

Since September'2015 Doctrine doesn't always save namespace version in
the cache itself:
doctrine/cache@ecc4af1

Relying on such kind of implementation detail in a test that is supposed
to check if several keys are saved in a cache is kind of pointless, so it
makes sense to ignore the cache key that holds a namespace version
@kamazee kamazee force-pushed the fix_cache_resolver_tests branch from 05a3c89 to 785a0d1 Compare October 30, 2015 19:31
@lsmith77
Copy link
Contributor

can you look at the prefer-lowest test too? https://travis-ci.org/liip/LiipImagineBundle/jobs/88415483
we might just have to bump up the lower dependencies

@kamazee
Copy link
Contributor Author

kamazee commented Oct 31, 2015

@lsmith77 yeah, I noticed that. That test fails with Imagine < 0.6, but it's not the code, it's the test that tests the code for old versions of Imagine improperly. I fixed the test and one ofther issue with AutoRotateFilterLoader (it didn't handle exceptional situations properly), see #656

@kamazee
Copy link
Contributor Author

kamazee commented Oct 31, 2015

So, when this PR and #657 are merged together, the build should get green. I'll probably put them together and create a dummy PR with both commits just to make Travis build them both and check my assumption.

@lsmith77 lsmith77 merged commit 785a0d1 into liip:master Nov 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants