Repository: JasonEtco/todo
Pull request #193 (2019-02-09 16:03:48)

Performance improvements
#192 surfaced some more realistic response time metrics, and operations can be pretty slow, especially for large diffs or PRs that change a lot of files. This PR paralellizes some requests using `Promise.all`. There is a case where one TODO can be duplicated in the same diff - I've added a test for that case and used `context.todos = []` to track issues generated in the same run.


Patch:
diff --git a/lib/push-handler.js b/lib/push-handler.js
index 8f09bd3..8c1780a 100644
--- a/lib/push-handler.js
+++ b/lib/push-handler.js
@@ -32,6 +32,7 @@ module.exports = async context => {
     // Prevent duplicates
     const existingIssue = await checkForDuplicateIssue(context, title)
     if (existingIssue) {
+      if (typeof existingIssue === 'string') return
       context.log(`Duplicate issue found with title [${title}]`)
       return reopenClosed({ context, config, issue: existingIssue }, { keyword, sha, filename })
     }
diff --git a/lib/utils/check-for-duplicate-issue.js b/lib/utils/check-for-duplicate-issue.js
index 93f9d4d..cb442f6 100644
--- a/lib/utils/check-for-duplicate-issue.js
+++ b/lib/utils/check-for-duplicate-issue.js
@@ -4,6 +4,9 @@
  * @param {string} title - An issue title
  */
 module.exports = async (context, title) => {
+  if (context.todos.includes(title)) return title
+  context.todos.push(title)
+
   const search = await context.github.search.issuesAndPullRequests({
     q: `${title} in:title repo:${context.payload.repository.full_name}`,
     per_page: 100
diff --git a/lib/utils/generate-label.js b/lib/utils/generate-label.js
index 0e6dfce..26fe0cf 100644
--- a/lib/utils/generate-label.js
+++ b/lib/utils/generate-label.js
@@ -11,7 +11,11 @@ module.exports = async (context, cfg) => {
   } else {
     if (cfg.label) {
       // Generate a label object
-      const newLabel = context.repo({name: 'todo :spiral_notepad:', color: '00B0D8'})
+      const newLabel = context.repo({
+        name: 'todo :spiral_notepad:',
+        color: '00B0D8',
+        request: { retries: 0 }
+      })
 
       // This will catch if the label already exists
       try {
diff --git a/lib/utils/main-loop.js b/lib/utils/main-loop.js
index 8a805e7..7fb49cf 100644
--- a/lib/utils/main-loop.js
+++ b/lib/utils/main-loop.js
@@ -7,48 +7,48 @@ const configSchema = require('./config-schema')
 const generateLabel = require('./generate-label')
 
 module.exports = async (context, handler) => {
-  // Grab the config file
-  const configFile = await context.config('config.yml')
-  const config = await configSchema.validate(configFile && configFile.todo ? configFile.todo : {})
+  context.todos = []
+
+  const [configFile, diff] = await Promise.all([
+    // Grab the config file
+    context.config('config.yml'),
+    // Get the diff for this commit or PR
+    getDiff(context)
+  ])
 
+  const config = await configSchema.validate(configFile && configFile.todo ? configFile.todo : {})
   const keywords = Array.isArray(config.keyword) ? config.keyword : [config.keyword]
 
   // Ensure that all the labels we need are present
   const labels = await generateLabel(context, config)
 
-  // Get the diff for this commit or PR
-  const diff = await getDiff(context)
-
   // RegEx that matches lines with the configured keywords
   const regex = new RegExp(`.*(?<keyword>${keywords.join('|')})\\s?:?(?<title>.*)`)
 
   // Parse the diff as files
   const files = parseDiff(diff)
 
-  for (const file of files) {
-    if (shouldExcludeFile(context.log, file.to, config.exclude)) continue
+  await Promise.all(files.map(async file => {
+    if (shouldExcludeFile(context.log, file.to, config.exclude)) return
 
     // Loop through every chunk in the file
-    for (const chunk of file.chunks) {
+    await Promise.all(file.chunks.map(async chunk => {
       // Chunks can have multiple changes
-      for (const indexStr in chunk.changes) {
-        const index = parseInt(indexStr, 10)
-        const change = chunk.changes[index]
-
+      await Promise.all(chunk.changes.map(async (change, index) => {
         // Only act on added lines
         // :TODO: Also handle deleted TODO lines
-        if (change.type !== 'add') continue
+        if (change.type !== 'add') return
 
         // Attempt to find a matching line: TODO Something something
         const matches = regex.exec(change.content)
-        if (!matches) continue
+        if (!matches) return
 
         // Trim whitespace to ensure a clean title
         const title = matches.groups.title.trim()
 
         // This might have matched a minified file, or something
         // huge. GitHub wouldn't allow this anyway, so let's just ignore it.
-        if (!title || title.length > 256) continue
+        if (!title || title.length > 256) return
 
         // Get the details of this commit or PR
         const deets = getDetails({ context, config, chunk, line: change.ln || change.ln2 })
@@ -57,7 +57,7 @@ module.exports = async (context, handler) => {
         context.log(`Item found [${title}] in [${owner}/${repo}] at [${deets.sha}]`)
 
         // Run the handler for this webhook listener
-        await handler({
+        return handler({
           keyword: matches.groups.keyword,
           bodyComment: checkForBody(chunk.changes, index, config),
           filename: file.to,
@@ -68,7 +68,7 @@ module.exports = async (context, handler) => {
           labels,
           ...deets
         })
-      }
-    }
-  }
+      }))
+    }))
+  }))
 }


Test patch:
diff --git a/tests/fixtures/diffs/duplicate.txt b/tests/fixtures/diffs/duplicate.txt
new file mode 100644
index 0000000..f258b84
--- /dev/null
+++ b/tests/fixtures/diffs/duplicate.txt
@@ -0,0 +1,23 @@
+diff --git a/.travis.yml b/.travis.yml
+index 0d43cbb..4e652e9 100644
+--- a/.travis.yml
++++ b/.travis.yml
+@@ -4,21 +4,22 @@
+language: node_js
+ 
+ node_js:
+   - "8"
+-  
++
+ notifications:
+   disabled: true
+-  
++ // TODO I am an example title
+ branches:
+   except:
+     - /^vd+.d+.d+$/
++ // TODO I am an example title
+-    
+ script:
+   - npm test
+   - codecov
\ No newline at end of file
diff --git a/tests/helpers.js b/tests/helpers.js
index f39c4b6..e394f91 100644
--- a/tests/helpers.js
+++ b/tests/helpers.js
@@ -34,7 +34,7 @@ exports.gimmeApp = () => {
 
   github = {
     issues: {
-      create: jest.fn().mockName('issues.create'),
+      create: jest.fn(data => Promise.resolve({ data })).mockName('issues.create'),
       createLabel: jest.fn().mockName('issues.createLabel'),
       update: jest.fn().mockName('issues.update'),
       createComment: jest.fn().mockName('issues.createComment'),
diff --git a/tests/lib/check-for-duplicate-issue.test.js b/tests/lib/check-for-duplicate-issue.test.js
new file mode 100644
index 0000000..c8d5a04
--- /dev/null
+++ b/tests/lib/check-for-duplicate-issue.test.js
@@ -0,0 +1,36 @@
+const checkForDuplicateIssue = require('../../lib/utils/check-for-duplicate-issue')
+const payload = require('../fixtures/payloads/push.json')
+
+describe('check-for-duplicate-issue', () => {
+  let context
+
+  beforeEach(() => {
+    context = {
+      payload,
+      github: {
+        search: {
+          issuesAndPullRequests: jest.fn(() => Promise.resolve({ data: { items: [] } }))
+        }
+      },
+      todos: []
+    }
+  })
+
+  it('returns undefined if no duplicate issue is found', async () => {
+    const actual = await checkForDuplicateIssue(context, 'hello')
+    expect(actual).toBe(undefined)
+  })
+
+  it('returns the issue if a duplicate issue is found in context.todos', async () => {
+    context.todos.push('hello')
+    const actual = await checkForDuplicateIssue(context, 'hello')
+    expect(actual).toBe('hello')
+  })
+
+  it('returns the issue if a duplicate issue is found by searching', async () => {
+    const mock = { data: { total_count: 1, items: [{ title: 'hello' }] } }
+    context.github.search.issuesAndPullRequests.mockReturnValueOnce(mock)
+    const actual = await checkForDuplicateIssue(context, 'hello')
+    expect(actual).toEqual(mock.data.items[0])
+  })
+})
diff --git a/tests/push-handler.test.js b/tests/push-handler.test.js
index 76af291..74f9e78 100644
--- a/tests/push-handler.test.js
+++ b/tests/push-handler.test.js
@@ -61,6 +61,12 @@ describe('push-handler', () => {
     expect(github.issues.create).not.toHaveBeenCalled()
   })
 
+  it('does not create the same issue twice in the same run', async () => {
+    github.repos.getCommit.mockReturnValueOnce(loadDiff('duplicate'))
+    await app.receive(event)
+    expect(github.issues.create).toHaveBeenCalledTimes(1)
+  })
+
   it('creates an issue if the search does not have an issue with the correct title', async () => {
     github.search.issuesAndPullRequests.mockReturnValueOnce(Promise.resolve({
       data: { total_count: 1, items: [{ title: 'Not found', state: 'open' }] }