Sample of LRU test refactor #1424
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wilhuff
reviewed
Jun 19, 2018
| @implementation FSTLRUGarbageCollectorTests { | ||
| FSTTargetID _previousTargetID; | ||
| int _previousDocNum; | ||
| FSTObjectValue *_testValue; | ||
| FSTObjectValue *_bigObjectValue; | ||
| } | ||
|
|
||
| - (TestResources)allocateResources { |
There was a problem hiding this comment.
"alloc" is special (meaning create but don't initialize). "new" would be better (and would match "newPersistence")
| @implementation FSTLRUGarbageCollectorTests { | ||
| FSTTargetID _previousTargetID; | ||
| int _previousDocNum; | ||
| FSTObjectValue *_testValue; | ||
| FSTObjectValue *_bigObjectValue; | ||
| } | ||
|
|
||
| - (TestResources)allocateResources { | ||
| id<FSTPersistence> persistence = [self newPersistence]; |
There was a problem hiding this comment.
Why not just make these objects members of the fixture and avoid the extra struct and the dance around it?
You could also assign _gc and avoid the [self gcForPersistence:] calls.
There was a problem hiding this comment.
I did it this way because of one test that creates a new persistence in a loop. Maybe I just need to separate out those test cases or something though.
There was a problem hiding this comment.
Eh, I figured out how to do it. everything is or will be a member variable now.
| }; | ||
| } | ||
|
|
||
| - (void)assertSequenceNumberOffsetAfterGC:(int)offset testResources:(const TestResources &)testResources { |
There was a problem hiding this comment.
This assertion will be painful when it fails because the file and line will be that of this function rather than the caller. If you need custom assertions it's best to make them macros as a result.
A better alternative would be to add a _gc member as described above and then add a helper that does
- (FSTListenceSequenceNumber)sequenceNumberForQueryCount:(int)count {
return _persistence.run("gc", [self]() {
return [_gc sequenceNumberForQueryCount:count];
});
}Then the caller can make assertions as it pleases.
gsoltis
force-pushed
the
gsoltis/lru_test_refactor
branch
from
7579c57 to
1901c6d
Compare
gsoltis
pushed a commit
that referenced
this pull request
Jul 18, 2018
* Bump sequence number on resume token refresh * Style * Fix comment formatting * Add FSTReferenceDelegate definition and documentation * Add methods to return nil for delegates, wire up inMemoryPins * Add hook for removing a reference * Start work on reference delegates * Fix up tests to support adding documents at a sequence number * Implement removing references * Remove from target when dropped from local view * Fix warning * Add hooks for removal from mutation queue * Add hooks for limbo document updates * Style * Drop commented-out code * Fixup after merging master * Start importing delegate implementations * Memory LRU tests pass * Make memory lru work, start fixing up lru tests. All with hidden sequence numbers * Most LRU tests passing w/ memory * Big LRU test passes w/ memory * Leveldb LRU tests pass * All passing except local store tests * All tests pass * Fix localstore tests * Revert removeQueryData change * Remove collectGarbage calls * Remove NoOp GC * More cleanup * Refactor FSTHelpers * Cleanup and commenting * Comments and cleanup * Comments * Drop nullable for reference delegates, and old persistence method on spec tests * Fix missing sequence bump, remove unneeded targetID * Style * Drop commented-out code * Satisfy the linters and copyrighters, etc. * Drop more commented-out code * Fix import -> include, NSUInteger -> int * Start work on test comments * FSTDocumentKey -> DocumentKey * Sample of LRU test refactor (#1424) * Sample of test refactor * Drop errant 'struct' keyword * More test refactoring * Make gc a member variable too * Tests refactored to use member variables * Assign delegates procedurally, rather than passing in a block * Fix up FSTLevelDB a little * listens -> listenTargets * Rename GCEnabled * remove isSentinel * reword comment * Add case for non-eager GC to test * Style * Cleaning the lint trap * Clean up LRU tests (#1433) * Add copious helpers and some docs to lru tests * Style * Method name refactor, add comment * Drop FSTGarbageCollector and FSTEagerGarbageCollector (#1440) * Drop FSTGarbageCollector and FSTEagerGarbageCollector * Drop unused containsKey from FSTLevelDBMutationQueue * Style
gsoltis
pushed a commit
that referenced
this pull request
Jul 27, 2018
* Switch to reference delegates for garbage collection (#1410) * Bump sequence number on resume token refresh * Style * Fix comment formatting * Add FSTReferenceDelegate definition and documentation * Add methods to return nil for delegates, wire up inMemoryPins * Add hook for removing a reference * Start work on reference delegates * Fix up tests to support adding documents at a sequence number * Implement removing references * Remove from target when dropped from local view * Fix warning * Add hooks for removal from mutation queue * Add hooks for limbo document updates * Style * Drop commented-out code * Fixup after merging master * Start importing delegate implementations * Memory LRU tests pass * Make memory lru work, start fixing up lru tests. All with hidden sequence numbers * Most LRU tests passing w/ memory * Big LRU test passes w/ memory * Leveldb LRU tests pass * All passing except local store tests * All tests pass * Fix localstore tests * Revert removeQueryData change * Remove collectGarbage calls * Remove NoOp GC * More cleanup * Refactor FSTHelpers * Cleanup and commenting * Comments and cleanup * Comments * Drop nullable for reference delegates, and old persistence method on spec tests * Fix missing sequence bump, remove unneeded targetID * Style * Drop commented-out code * Satisfy the linters and copyrighters, etc. * Drop more commented-out code * Fix import -> include, NSUInteger -> int * Start work on test comments * FSTDocumentKey -> DocumentKey * Sample of LRU test refactor (#1424) * Sample of test refactor * Drop errant 'struct' keyword * More test refactoring * Make gc a member variable too * Tests refactored to use member variables * Assign delegates procedurally, rather than passing in a block * Fix up FSTLevelDB a little * listens -> listenTargets * Rename GCEnabled * remove isSentinel * reword comment * Add case for non-eager GC to test * Style * Cleaning the lint trap * Clean up LRU tests (#1433) * Add copious helpers and some docs to lru tests * Style * Method name refactor, add comment * Drop FSTGarbageCollector and FSTEagerGarbageCollector (#1440) * Drop FSTGarbageCollector and FSTEagerGarbageCollector * Drop unused containsKey from FSTLevelDBMutationQueue * Style * Include sequence number in FSTQueryData hash and isEqual (#1568) * Include sequence number in FSTQueryData hash and isEqual methods * Style * Switch to util::Hash * Fix preserving query cache metadata (#1569) * Add failing test * Move the restart tests to the leveldb test suite * Style * Add new persistence test helper * Fix test failure * Review feedback * Style
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@wilhuff let me know what you think of this kind of refactor, and I'll go ahead and make the change for the rest of the tests.