Repository: ViacomInc/data-point
Pull request #113 (2018-01-04 18:00:58)

fix(data-point-cache/in-memory): refactor code to make tests reliable
<!--
Thanks for your interest in the project. We appreciate bugs filed and PRs submitted!

Please make sure that you are familiar with and follow the Code of Conduct for
this project (found in the CODE_OF_CONDUCT.md file).

Also, please make sure you're familiar with and follow the instructions in the
contributing guidelines (found in the CONTRIBUTING.md file).

Please fill out the information below to expedite the review and (hopefully)
merge of your pull request!
-->

<!-- What changes are being made? (What feature/bug is being fixed here?) -->
**What**: refactor code to make test reliable

<!-- Why are these changes necessary? -->
**Why**: test were randomly failing due to how it relied on `setInterval`

<!-- How were these changes implemented? -->
**How**: isolating swipe logic so it can be unit tested independently of its implementation 

<!-- Have you done all of these things?  -->
**Checklist**:
<!-- add "N/A" to the end of each line that's irrelevant to your changes -->
<!-- to check an item, place an "x" in the box like so: "- [x] Documentation" -->
- [N/A ] Documentation
- [X] Tests
- [X] Ready to be merged <!-- In your opinion, is this ready to be merged as soon as it's reviewed? -->
- [ ] Added username to **all-contributors** list

<!-- feel free to add additional comments -->


coveralls: 
[![Coverage Status](https://coveralls.io/builds/14903237/badge)](https://coveralls.io/builds/14903237)

Coverage remained the same at 100.0% when pulling **a1d718dcefb79baf8936460df4e03984357c3c4c on acatl:cache-in-memory-fix-tests** into **825f9a3b9c1cc7dd2f627801e9411718cbc91e31 on ViacomInc:master**.

coveralls: 
[![Coverage Status](https://coveralls.io/builds/14903240/badge)](https://coveralls.io/builds/14903240)

Coverage remained the same at 100.0% when pulling **a1d718dcefb79baf8936460df4e03984357c3c4c on acatl:cache-in-memory-fix-tests** into **825f9a3b9c1cc7dd2f627801e9411718cbc91e31 on ViacomInc:master**.

acatl: @paulmolluzzo, I applied your requests
coveralls: 
[![Coverage Status](https://coveralls.io/builds/14905179/badge)](https://coveralls.io/builds/14905179)

Coverage remained the same at 100.0% when pulling **5de2235c926f379fe406503dc9f08fdd9a1f1b3e on acatl:cache-in-memory-fix-tests** into **825f9a3b9c1cc7dd2f627801e9411718cbc91e31 on ViacomInc:master**.


Patch:
diff --git a/packages/data-point-cache/lib/in-memory.js b/packages/data-point-cache/lib/in-memory.js
index c7a8f36..291978a 100644
--- a/packages/data-point-cache/lib/in-memory.js
+++ b/packages/data-point-cache/lib/in-memory.js
@@ -20,27 +20,33 @@ function del (cache, key) {
   delete cache.entries[key]
 }
 
+/**
+ * NOTE: This method mutates the cache.entires array.
+ * @param {Object} cache - Object that holds a reference to list of cache entries
+ */
+function swipeTick (cache) {
+  const keys = Object.keys(cache.entries)
+  if (keys.length > 10000) {
+    cache.entries = {}
+    logger.warn(
+      'Cache inMemory reached max (10000) number of entries, all keys now being deleted.'
+    )
+    return
+  }
+
+  const now = Date.now()
+  for (let index = 0; index < keys.length; index++) {
+    let key = keys[index]
+    let entry = cache.entries[key]
+    if (now - entry.created > entry.ttl) {
+      delete cache.entries[key]
+    }
+  }
+}
+
 function swipe (cache, interval = 1000) {
   clearInterval(cache.swipeTimerId)
-  cache.swipeTimerId = setInterval(() => {
-    const keys = Object.keys(cache.entries)
-    if (keys.length > 10000) {
-      cache.entries = {}
-      logger.warn(
-        'inMemory reached max (1000) number of entries, all keys now being deleted.'
-      )
-      return
-    }
-    let entry
-    const now = Date.now()
-    for (let index = 0; index < keys.length; index++) {
-      let key = keys[index]
-      entry = cache.entries[key]
-      if (now - entry.created > entry.ttl) {
-        delete cache.entries[key]
-      }
-    }
-  }, interval)
+  cache.swipeTimerId = setInterval(swipeTick, interval, cache)
   return cache.swipeTimerId
 }
 
@@ -68,6 +74,7 @@ module.exports = {
   set,
   get,
   del,
+  swipeTick,
   swipe,
   create,
   bootstrap


Test patch:
diff --git a/packages/data-point-cache/lib/in-memory.test.js b/packages/data-point-cache/lib/in-memory.test.js
index c8102cc..553c896 100644
--- a/packages/data-point-cache/lib/in-memory.test.js
+++ b/packages/data-point-cache/lib/in-memory.test.js
@@ -39,63 +39,49 @@ describe('del', () => {
   })
 })
 
-describe('swipe', () => {
+describe('swipeTick', () => {
   let cache
   beforeEach(() => (cache = { entries: {} }))
-  test('it should remove keys - testing swipes', done => {
-    cache.entries['test1'] = { value: 'test1', ttl: 10, created: Date.now() }
-    cache.entries['test2'] = { value: 'test2', ttl: 60, created: Date.now() }
-
-    const timer = InMemory.swipe(cache, 10)
-
-    const swipes = []
-    setTimeout(() => {
-      swipes.push(Object.keys(cache.entries))
-    }, 5)
-
-    setTimeout(() => {
-      swipes.push(Object.keys(cache.entries))
-    }, 30)
-
-    setTimeout(() => {
-      swipes.push(Object.keys(cache.entries))
-    }, 80)
-
-    setTimeout(() => {
-      let error
-      try {
-        expect(swipes[0]).toHaveLength(2)
-        expect(swipes[1]).toHaveLength(1)
-        expect(swipes[2]).toHaveLength(0)
-      } catch (err) {
-        error = err
-      } finally {
-        clearInterval(timer)
-        done(error)
-      }
-    }, 500)
-  })
-  test('it should remove all keys if length is more than permited', done => {
+  test('it should remove all keys if length is more than permited', () => {
     const entry = { value: 'test1', ttl: 1000, created: Date.now() }
     for (var index = 0; index < 10001; index++) {
       cache.entries[`${index}key`] = entry
     }
 
-    const timer = InMemory.swipe(cache, 10)
     logger.warn = jest.fn()
-    setTimeout(() => {
-      let error
-      try {
-        const keys = Object.keys(cache.entries)
-        expect(keys).toHaveLength(0)
-        expect(logger.warn).toBeCalled()
-      } catch (err) {
-        error = err
-      } finally {
-        clearInterval(timer)
-        done(error)
-      }
-    }, 100)
+    InMemory.swipeTick(cache)
+    const keys = Object.keys(cache.entries)
+    expect(keys).toHaveLength(0)
+    expect(logger.warn).toBeCalled()
+  })
+
+  test('it should remove keys - on each tick', () => {
+    let now = 0
+
+    cache.entries['test1'] = { value: 'test1', ttl: 10, created: now }
+    cache.entries['test2'] = { value: 'test2', ttl: 20, created: now }
+
+    const originalDateNow = Date.now
+
+    const swipes = []
+
+    Date.now = () => 5
+    InMemory.swipeTick(cache)
+    swipes.push(Object.keys(cache.entries))
+
+    Date.now = () => 11
+    InMemory.swipeTick(cache)
+    swipes.push(Object.keys(cache.entries))
+
+    Date.now = () => 21
+    InMemory.swipeTick(cache)
+    swipes.push(Object.keys(cache.entries))
+
+    expect(swipes[0]).toHaveLength(2)
+    expect(swipes[1]).toHaveLength(1)
+    expect(swipes[2]).toHaveLength(0)
+
+    Date.now = originalDateNow
   })
 })