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

Adding symfony cache #62

Merged
merged 9 commits into from
Jul 10, 2023
Merged

Adding symfony cache #62

merged 9 commits into from
Jul 10, 2023

Conversation

Samuel-Gill
Copy link
Contributor

@Samuel-Gill Samuel-Gill commented Jul 5, 2023

In this PR, I am adding symfony cache.

Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

There's a lot of trivial issues here that could be causing it. Just mismatching interfaces and stuff.

src/cache/DefaultCache.php Outdated Show resolved Hide resolved
src/cache/DefaultCache.php Outdated Show resolved Hide resolved
src/cache/DefaultCache.php Outdated Show resolved Hide resolved
src/cache/DefaultCache.php Outdated Show resolved Hide resolved
src/cache/DefaultCache.php Outdated Show resolved Hide resolved
@Samuel-Gill
Copy link
Contributor Author

image

@UmanShahzad all tests has passed. Please review.

src/cache/DefaultCache.php Outdated Show resolved Hide resolved
src/cache/DefaultCache.php Outdated Show resolved Hide resolved
tests/DefaultCacheTest.php Outdated Show resolved Hide resolved
tests/DefaultCacheTest.php Outdated Show resolved Hide resolved
src/cache/DefaultCache.php Outdated Show resolved Hide resolved
Copy link
Contributor

@UmanShahzad UmanShahzad left a comment

Choose a reason for hiding this comment

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

@Samuel-Gill have we shown this to work with the test cases? And do we have valid test cases for testing the cache, like repeating the same lookup a few times?

@Samuel-Gill
Copy link
Contributor Author

@Samuel-Gill have we shown this to work with the test cases? And do we have valid test cases for testing the cache, like repeating the same lookup a few times?

Yes I have run tests present in tests/DefaultCacheTest.php . It all passed. If any new test is required which is not covering our use-case, you can let me know and I'll create it.

@UmanShahzad
Copy link
Contributor

@Samuel-Gill I don't know - you gotta check yourself and see if the tests cover the changes made here well enough!

@UmanShahzad
Copy link
Contributor

Also CI is failing.

@Samuel-Gill Samuel-Gill changed the title [WIP] - Adding symfony cache Adding symfony cache Jul 10, 2023
@Samuel-Gill
Copy link
Contributor Author

@Samuel-Gill I don't know - you gotta check yourself and see if the tests cover the changes made here well enough!

Yes I have checked, we are covering the changes made here.
We are running following test:

  • testHasValue
  • testDoesNotHaveValue
  • testGetValue
  • testMaxSizeExceeded
  • testTimeToLiveExceeded

@Samuel-Gill
Copy link
Contributor Author

Also CI is failing.

Fixed.

@UmanShahzad UmanShahzad merged commit 79e23a9 into master Jul 10, 2023
3 checks passed
@UmanShahzad UmanShahzad deleted the symfony-cache-update branch July 10, 2023 08:21
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.

None yet

2 participants