L1 tests refactoring by ndobromirov · Pull Request #76 · lcache/lcache

ndobromirov · 2016-12-27T16:46:11Z

Overview

This pull request implements the described L1 testing changes in #43 for APCu, Static and SQLite drivers.

Summary

There is a new L1CacheTest abstract base class.
There are 3 successors for it, 1 for each driver (APCu, Static, SQLite).

There were some incompatibilities between L1 Implementations and tests:

Pool sharing between instances was tested only for SQLite. Whe ported to test against all L1s, the tests for the static implementation failed. This is fixed in the driver, so all are currently passing.
APCu had a comment that it clears the whole APCu data on flush. This is re-implemented with an iterator, so now it should be safe.

This is based on top of #73, as the changes here depend on:

Unified L1 creation interface.
Factory for all L1 drivers.

Description

All the pure L1 tests are moved to the L1 testing hierarchy.
TODO: L2 and Integrations.

…ween L1 cache implementations and their respective state handlers.

…y instantiating L1 concrete classes.

…e manager.

…thods on the StateL1Null class.

…tored 2 loops in StaticL2 to reduce code indentation.

…he#75)

…ial L1 concrete classes for APCu, Static and SQLite.

…s to reduce danger in using the library (maybe added overhead on cache clear).

…l L1 drivers. Had to Refactor StaticL1 for that.

coveralls · 2016-12-27T17:01:10Z

Coverage remained the same at 100.0% when pulling 5ba363a on ndobromirov:l1-tests-refactoring into a11946d on lcache:master.

davidstrauss

This is getting very, very close.

davidstrauss · 2016-12-27T17:32:49Z

src/L1CacheFactory.php

+     * @param string $pool
+     * @return \LCache\APCuL1
+     */
+    protected function createApcu($pool)


Should we call this createAPCu?

davidstrauss · 2016-12-27T17:33:09Z

src/L1CacheFactory.php

+     * @param string $pool
+     * @return \LCache\SQLiteL1
+     */
+    protected function createSqlite($pool)


Should we call this createSQLite?

davidstrauss · 2016-12-27T17:34:34Z

src/StateL1APCu.php

+     * @return bool
+     *   True on success, false otherwise.
+     */
+    private function _recordEvent($key)


Is there a reason this has an underscore prefix?

It's a personal preference. I like to have all private stuff prefixed with underscore old habit left from development on Yii framework. If it's not ok with the project's naming conventions it fairly easy to change. Though the code sniffer did not complained for that :).

Done. Also changed the new static variable in the static cache instance.

davidstrauss · 2016-12-27T17:36:36Z

src/StateL1APCu.php

+    {
+        apcu_store($this->statusKeyHits, 0);
+        apcu_store($this->statusKeyMisses, 0);
+        // TODO: Decide on how to handle the last applied event state on clear?


I think resetting statistics and resetting the last applied event ID should be separate. The latter should, generally, only get flushed when the entire local cache gets flushed.

So it's ok as it is. I'll just remove the comment.

davidstrauss · 2016-12-27T17:39:52Z

src/StateL1Null.php

+ *
+ * @author ndobromirov
+ */
+class StateL1Null extends StateL1Static implements StateL1Interface


I'm concerned about how this splits the PHP_INT_MAX for getLastAppliedEventID away from NullL1. It allows incorrect behavior when someone uses this state implementation with a stateful L1. As a rule, it should be possible to combine any StateL1 with any L1 and have correct (but maybe not ideal) behavior.

Perhaps, we should just not have a StateL1Null and use StateL1Static with NullL1 when testing.

I felt something was off when I was implementing this. The only state implementation that caused issues was the NULL one. I think that state should be ok to be the static one. Will change it so.

State NULL is deleted. On all places the static one is used now. The PHP_INT_MAX was moved back to the NullL1.

…composer.

coveralls · 2016-12-27T20:11:41Z

Coverage remained the same at 100.0% when pulling d011c69 on ndobromirov:l1-tests-refactoring into a11946d on lcache:master.

ndobromirov · 2016-12-27T20:20:54Z

I think I've missed the NullL1 cache to have it's own L1 test class.

davidstrauss · 2016-12-27T20:29:24Z

I think I've missed the NullL1 cache to have it's own L1 test class.

I'm not sure what you mean. Should I hold off on merging until you make some more changes?

coveralls · 2016-12-27T20:38:38Z

Coverage remained the same at 100.0% when pulling 60af8a8 on ndobromirov:l1-tests-refactoring into a11946d on lcache:master.

ndobromirov · 2016-12-27T22:21:22Z

That's it for L1s (for now).

I have started working on the L2 but it is going to be tricky there, as I am still getting to know the API and tests for it.

You can decide how to do it.

Merge as it is now and have L2 + integration added as separate PRs.
One huge PR (so this will get bigger).

davidstrauss · 2016-12-27T22:21:56Z

Merge as it is now and have L2 + integration added as separate PRs.

I always prefer working incrementally.

ndobromirov and others added 22 commits December 9, 2016 15:49

Factored-out the state management in designated classes.

e66ddfb

Removed excessive logic in L1 constructor. Added hard dependecies bet…

16def54

…ween L1 cache implementations and their respective state handlers.

Added docks v1. Added null state manager implementation.

d513eaa

Refacrored the tests to use the new L1CacheFactory instead of directl…

3626134

…y instantiating L1 concrete classes.

Improvements in StateL1Null implementaion and fixes for scrutinizer CI.

3d4d54d

CS fixes and improvements against scrutnizer.

085c651

Removed dead code.

8b0040e

Tweak in the L1 Null caches implementation together with L1 Null stat…

247b398

…e manager.

Added a test for the invalid driver on the factory. Removed unused me…

341377a

…thods on the StateL1Null class.

CS fixes.

c4419bd

Inevrted === and !== checks to have constant as first argument. Refac…

1fb9130

…tored 2 loops in StaticL2 to reduce code indentation.

SQLiteL1: Determine filename based on schema definition strings (lcac…

cecb2a4

…he#75)

Refactorin in L1 related tests. Abstracted L1 tests. Implemented Init…

7eef3a8

…ial L1 concrete classes for APCu, Static and SQLite.

Moved performL1AntirollbackTest to the new L1 tests structure.

3f1e10f

Refactored performL1FullDelete. Implemented a TODO from the APCu clas…

459d238

…s to reduce danger in using the library (maybe added overhead on cache clear).

Refactored L1 expiration tests.

96f5c2b

Refacored pool and existance tests.

83f22bd

Refacored L1 hit/miss test.

2bccef6

Refactored the testSQLiteL1SchemaErrorHandling to be triggered for al…

0596626

…l L1 drivers. Had to Refactor StaticL1 for that.

Added key-overhead tracking tests on L1 level.

006bfcb

Added existence and validity checks for running the APCu tests.

73a8a6b

CS fixes & re-formatting.

5ba363a

davidstrauss self-requested a review December 27, 2016 17:31

davidstrauss added the enhancement label Dec 27, 2016

davidstrauss added this to the v1.0.0 milestone Dec 27, 2016

davidstrauss requested changes Dec 27, 2016

View reviewed changes

Fixed the CRs from review. Added tests folder under CS validation in …

d011c69

…composer.

davidstrauss approved these changes Dec 27, 2016

View reviewed changes

Implemented a separate test class for the NullL1

60af8a8

davidstrauss merged commit 275282b into lcache:master Dec 27, 2016

ndobromirov deleted the l1-tests-refactoring branch December 27, 2016 22:29

ndobromirov mentioned this pull request Dec 27, 2016

Factored-out the state management in designated classes. #73

Closed

2 tasks

stevector mentioned this pull request Dec 28, 2016

(WIP) Refactor L1 tests out of LCacheTest.php and into separate test classes #72

Closed

5 tasks

ndobromirov mentioned this pull request Jan 8, 2017

Separate L1 replication/hit/miss tracking from storage #63

Open

Conversation

ndobromirov commented Dec 27, 2016

Overview

Summary

Description

Uh oh!

coveralls commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidstrauss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndobromirov commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidstrauss commented Dec 27, 2016

Uh oh!

coveralls commented Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndobromirov commented Dec 27, 2016

Uh oh!

davidstrauss commented Dec 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coveralls commented Dec 27, 2016 •

edited

Loading

coveralls commented Dec 27, 2016 •

edited

Loading

ndobromirov commented Dec 27, 2016 •

edited

Loading

coveralls commented Dec 27, 2016 •

edited

Loading