L2 & Integrated - tests refactoring by ndobromirov · Pull Request #79 · lcache/lcache

ndobromirov · 2016-12-30T16:39:52Z

Overview

This pull request is continuing the work on tests outlined in #43, namely L2 and the Integration parts.
All the tests from LCacheTest class are ported to the new structure.
The whole thing is open for reviews.

Additional things included:

All the tests for the integration layer are done.
All the integration tests are run against all the combinations of L1 and L2 drivers.
Many compatibility fixes between the StaticL2 and DatabaseL2.
Improved queued delete on DatabaseL2 - enforced consistency instead of eventual.
Improved some SQL queries (many into one) on DatabaseL2.
Full test coverage of the state L1 drivers. All are compliant, after some tweaks.
Improved L1 cache tests to be run against all StateL1 implementations.
Fixed many TODOs left in the code.
Improved multiple in-line docks.
Tests segregation in groups: state, l1, l2, integration.

It would be nice to have reviews at some point, as I will not have much time to fix stuff starting from next week :).

Summary

There is a new L2CcheTest base class. It is used for generic L2 tests. Specific ones are implemented in either the tests/L2/StaticTest (0) or tests/L2/DatabaseTest (2) classes.

All quirks needed to have DB connection and schema are implemented in a trait and moved out of the new test classes. This trait should be reusable on the Integration layer tests in near future.

There is a new factory class that is to be used with L2 hierarchy. POC for using the factory is done on the newly created tests.

Successfully moved tests to the new L2 structure are removed from the LCacheTest class.

Current implementation follows this idea:

StateL1 drivers are fully compliant with each other.
All CacheL1 are tested against all StateL1s, so every combination works consistently.
All CacheL2 are tested against all CacheL1 (with the recommended default state for it). This way we are reducing combinatory complexity for the tests, without sacrificing the stability much.

Issues faced (for now)

FIXED The method DatabaseTest::testCleanupAfterWrite is fully L2 based, but not compatible with the StaticL2 driver. When I've attempted to make the static compatible with it I faced the issue described in the comment from issue Refactoring tests. #43. Personally I think this should be fixed, so all L2 drivers will be enforced as much as possible common tests. The static driver now clears old events on an address.
FIXED There are too few L2-only tests in the code-base (compared to L1). Most of the tests left in LCacheTest class are covering the Integration layer and thus the L2 drivers partially, as they are a sub-system there. The problem here is that L2 is more complex than L1 and it has less tests purely for it. This is likely the issue that caused the the divergence between Static and Database L2 implementations.
FIXED Pick a direction, both are done. There is an inconsistency between L2 implementations of L2::applyEvents. To comply with the old tests at the moment they are both behaving as the StaticL2 - returning the last event ID always. For DB though this is a performance regression, as it will cause a bit more data to be read form the DB, as it includes the current pool (once). There is a branch that both L2 instances behave as the optimal DatabaseL2 (same as in master) here, but then there is the inconsistency for sync of local pools and remote ones. We need to decide how to proceed. Personally I prefer the solution present, where both are as the StaticL2, as the final API is more logical. When applyEvents is called the L1 will have the latest event as current, without considering locality. The overhead will be smaller and smaller based on the amount of servers handling L1. The read overhead is only on the first sync.
NOT RELATED (open a separate issue / PR for it) Decide a direction for a fix on LCache\L1\APCuTest::testNegativeCache as the APCu is the only incompatible implementation there. This was left as is, as it is not part of the tests refactoring but an edge case of the APCu driver for L1. Should be fixed separately. The refactoring on tests just uncovered it.

…es to do the clean-up. Tweaks in readability of the code.

…comply better to the DatabaseL2 one.

ndobromirov · 2017-01-04T15:02:22Z

composer.json

        "sami-install": "mkdir -p $HOME/bin && curl --output $HOME/bin/sami.phar http://get.sensiolabs.org/sami.phar && chmod +x $HOME/bin/sami.phar",
-        "test": "phpunit"
+        "test": "phpunit",
+        "test-failing": "phpunit --group failing"


This was added, so when a new annotation is added to a test like:
@group failing
The composer test-failing command will trigger only that test.
Useful when debugging a single test.

…sses.

…e code reusage.

…gativeCache check.

…L1's. APCu is failing.

ndobromirov · 2017-01-04T21:29:49Z

src/DatabaseL2.php

+            $sth = $this->dbh->prepare($sql);
+            foreach (array_keys($this->address_delete_queue) as $i => $address) {
+                $event_id = $this->address_delete_queue[$address];
+                $sth->bindValue($i * 2 + 1, $event_id, \PDO::PARAM_INT);


A very small micro-optimisation could be to swap th *2 to <<1.

…nd DatabaseL2.

…incompatability. Fixed an issue in DatabaseL2.

ndobromirov · 2017-01-05T16:10:37Z

src/DatabaseL2.php

            $sql = 'SELECT "event_id", "pool", "address", "value", "created", "expiration"'
                . ' FROM ' . $this->eventsTable
                . ' WHERE "event_id" > :last_applied_event_id'
-                . ' AND "pool" <> :exclude_pool'


This was changed to make both L2 drivers to behave consistently.
Though this change is causing small performance regressions, it is behaving as the StaticL2 does at the moment.
There is a non-merged implementation that makes them like the DB here

I will maintain both branches until it is decided how L2::applyEvents should behave and merge the correct version.

I will review and decide today.

… delete method. Improved docks.

…e L2 logical implementation.

…incompatability in StaticL2.

…rovements in DatabaseL2::exists db query.

ndobromirov · 2017-01-06T10:00:03Z

The disablement will cause that the parameter to be ignored and collect ALL garbage instead of only LIMIT items.

…ch as possible. Overloaded for DatabaseL2 test to pass.

…e factory test.

…r the delete in DatabaseL2::set, as it will fail on the insert first.

…ock.

coveralls · 2017-01-08T15:44:23Z

Coverage remained the same at 100.0% when pulling b9e4118 on ndobromirov:l2-tests-refactoring into 275282b on lcache:master.

ndobromirov · 2017-01-08T15:55:49Z

src/Integrated.php


 namespace LCache;

-final class Integrated


many of the Integration level tests were implemented by knowing the inner workings of L1 and L2 and tweaking the inner objects directly, ruining the encapsulation idea of the class and also consistent factory interface implementation for all the tests.

By removing the final here I did a descendant (IntegratedMock) that added many of the needed accessors to port the tests as close as possible.

ndobromirov · 2017-01-08T15:58:13Z

src/L1CacheFactory.php

-        $state = $hasApcu ? new StateL1APCu("sqlite-$pool") : new StateL1Static();
-        $cache = new SQLiteL1($pool, $state);
-        return $cache;
+        $stateDriver = function_exists('apcu_fetch') ? 'apcu' : 'sqlite';


Added APCu and SQLite as this are the ones that need to be done at some point. I am trusting the fall-back in the factory to instantiate static instead of SQLite state manager for now.

ndobromirov · 2017-01-08T16:01:13Z

src/StaticL2.php

+     * @return boolean
+     *   True on success.
+     */
+    public function pruneReplacedEvents()


Consider moving this to the base L2 class.

ndobromirov · 2017-01-08T16:04:42Z

tests/EntryTest.php

+        $this->assertFalse($this->getEntry(1)->isExpired());
+
+        // TODO: How to classify this one?
+        $this->assertFalse($this->getEntry(0)->isExpired());


Is expired and getTTL are a bit strange for relative times. They are the same for older and newer times, bu the difference comes from the current moment in time. The function getTTL says it has no time to live (but it is not dead yet) and the isExpired follows the same idea at the moment.

I would prefer to remove the concept of TTLs everywhere possible. They're harder to reason about and test than plain expiration timestamps.

ndobromirov · 2017-01-08T16:07:59Z

tests/Utils/LCacheDBTestTrait.php

+            . '"created" INTEGER NOT NULL)');
+        $this->dbh->exec('CREATE INDEX ' . $prefix . 'latest_entry ON ' . $prefix . 'lcache_events ("address", "event_id")');
+
+        // @TODO: Set a proper primary key and foreign key relationship.


Is this still relevant? There are already PK and FK and an index.

Yes. It's to establish a proper CASCADE DELETE. If you don't have that, tags on expired and deleted items will currently persist. That's fine for testing (as you won't get incorrect data), but it would leave orphaned data in production.

To fix it, we need to add REFERENCES lcache_events(event_id) ON DELETE CASCADE (observing $prefix for lcache_events) to the column lcache_tags.event_id.

For the tests it's already there some lines below... So is the TODO still relevant?
FOREIGN KEY("event_id") REFERENCES ' . $prefix . 'lcache_events("event_id") ON DELETE CASCADE)');
The cascade delete is used and proven working in the tag delete tests for DatabaseL2 here in this PR.

davidstrauss · 2017-01-11T04:21:32Z

I'm doing a bunch of work on this now, including some major refactoring. It's mostly hierarchical namespaces and isolating the synchronization status from hit/miss statistics. I'm working from the branch for this pull request (taken earlier today), but it might be tough to incorporate other changes until I've finished the work. Just letting you know.

ndobromirov · 2017-01-11T11:58:20Z

Nice to hear :) that the PR is going in, one way or the other.

ndobromirov · 2018-07-09T12:28:13Z

Are your changes ready?

Any chances for this to be merged at some point?

If so, we can push this a bit more and make Memcached / Redis L1 drivers and make this one a production ready library.

ndobromirov added 6 commits December 28, 2016 01:46

Refactoring in DatabaseL2 for readability. Added TODOs and utils.

419cdd8

Kick-off on the L2 tests refactoring.

047851e

CS fixes.

6b7a3f3

Fix code coverage. Added some docks.

43b389a

Code coverage iteration.

e378fc7

Improved delete queue consistency to avoide reliance of other process…

d30b6da

…es to do the clean-up. Tweaks in readability of the code.

ndobromirov mentioned this pull request Jan 4, 2017

Are duplicate database sets/clears never cleaned up? #67

Open

5 tasks

Implemented clean-up after write for the StaticL2 driver, so it will …

b4f91c0

…comply better to the DatabaseL2 one.

ndobromirov commented Jan 4, 2017

View reviewed changes

ndobromirov added 6 commits January 4, 2017 18:02

Imrpved code reusage in DatabaseL2. Added docks to LX and L2 base cla…

239cd11

…sses.

Reflow of the integrated layer. Added some inline docks.

e712eb6

DatabaseL2: single query to insert cache tags. Refactoring to increas…

4f1d07f

…e code reusage.

Added new Entity::isExpired. Used it to strenghten the StaticL1::isNe…

0068125

…gativeCache check.

Refactored SQLiteL1 sql statments for readability.

f0b649f

Added a new test to validate L1::isNegativeCache consistency for all …

12cc277

…L1's. APCu is failing.

ndobromirov commented Jan 4, 2017

View reviewed changes

ndobromirov added 2 commits January 5, 2017 16:17

Added a test for L2::applyEvents. Fixed incompatability in StaticL2 a…

cb72108

…nd DatabaseL2.

Implemented a dedicated L2::deleteTag test. Fixed StaticL2 interface …

cbba6dc

…incompatability. Fixed an issue in DatabaseL2.

ndobromirov commented Jan 5, 2017

View reviewed changes

ndobromirov added 3 commits January 5, 2017 19:08

Removed code coverage ignore tags from 2 methods in StaticL2.

3ddc2af

Moved shared logick from StaticL2 and DatabaseL2 to L2 abstract - the…

d530702

… delete method. Improved docks.

Micro-optimisations. Re-ordered code in StaticL2 to match the Databas…

0ffea2d

…e L2 logical implementation.

ndobromirov mentioned this pull request Jan 5, 2017

Refactoring tests. #43

Open

ndobromirov added 3 commits January 6, 2017 10:36

Added a test for the L2 garbage collection related interfaces. Fixed …

64a0285

…incompatability in StaticL2.

Improved consistency between StaticL2 and DatabaseL2. Performance imp…

82db81b

…rovements in DatabaseL2::exists db query.

Disabled DatabaseL2::collectGarbage limit input for all SQLite storage.

ae09451

ndobromirov added 2 commits January 6, 2017 12:41

Refactor the L2::collectGarbage test to enforce the correct API as mu…

6557519

…ch as possible. Overloaded for DatabaseL2 test to pass.

Audit the TODOs in code. Removed the ones that are not relevant anymore.

05ef967

ndobromirov mentioned this pull request Jan 6, 2017

Disable comments on PRs from Coveralls tool. #82

Closed

1 task

ndobromirov added 11 commits January 8, 2017 14:55

Ported the test for broken DatabaseL2 used in the integration layer.

bda8deb

Integration: Ported the test - Sync with no writes.

c4397c8

Integrated: Ported the existence test.

f607675

Integrated: Ported the poll value access test.

9d56dfe

Integrated: Ported expiration test.

a34e97d

Integrated: Ported excessive overhead skipping test.

67512a1

Integrated: Ported hit-set-counter (key overhead) test.

ea61e23

Integrated: Ported the failed unserialization test.

501c951

Integrated: Ported the garbage colletion tests.

8a5b9ca

Integrated: Ported tests for wrong serialization cases. L1: Ported th…

a5088a0

…e factory test.

Removed the LcacheTest class as obsolete (no tests are left in it).

8f937d7

ndobromirov changed the title ~~L2 tests refactoring~~ L2 & Integrated - tests refactoring Jan 8, 2017

ndobromirov added 3 commits January 8, 2017 17:22

Fixed code coverage (iteration 1).

a32dba5

Fixed code coverage (iteration 2). Reverted the exception handling fo…

52b2041

…r the delete in DatabaseL2::set, as it will fail on the insert first.

Fixed the Integrated API hacks. Moved them to the wrapper IntegratedM…

b9e4118

…ock.

ndobromirov commented Jan 8, 2017

View reviewed changes

This was referenced Jan 8, 2017

Improve comprehensibility of the codebase #74

Open

New State driver implementation on top of SQLite #80

Open

davidstrauss self-assigned this Jan 10, 2017

davidstrauss added the enhancement label Jan 10, 2017

davidstrauss added this to the v1.0.0 milestone Jan 10, 2017

davidstrauss removed their assignment Jan 10, 2017

Conversation

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

Uh oh!

Overview

Summary

Issues faced (for now)

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!

ndobromirov commented Jan 6, 2017

Uh oh!

coveralls commented Jan 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

davidstrauss Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndobromirov Jan 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidstrauss commented Jan 11, 2017

Uh oh!

ndobromirov commented Jan 11, 2017

Uh oh!

ndobromirov commented Jul 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ndobromirov commented Dec 30, 2016 •

edited

Loading

coveralls commented Jan 8, 2017 •

edited

Loading

davidstrauss Jan 10, 2017 •

edited

Loading

ndobromirov Jan 11, 2017 •

edited

Loading