From 038c5f583e39eacabc11e3552ffa02b8f40b4604 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Oct 2025 07:16:42 +0000 Subject: [PATCH 1/6] Initial plan From 1d51e32ba7039f0c12849e0a4e7ab1e657fc9857 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Oct 2025 07:29:06 +0000 Subject: [PATCH 2/6] Add bot startup test to prevent deploying unstartable bots Co-authored-by: rhamenator <51587867+rhamenator@users.noreply.github.com> --- spec/bot-startup-spec.js | 111 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 spec/bot-startup-spec.js diff --git a/spec/bot-startup-spec.js b/spec/bot-startup-spec.js new file mode 100644 index 0000000..fdca1f1 --- /dev/null +++ b/spec/bot-startup-spec.js @@ -0,0 +1,111 @@ +const { spawn } = require('child_process') +const path = require('path') + +describe('bot startup', () => { + // Set a reasonable timeout for bot startup + jasmine.DEFAULT_TIMEOUT_INTERVAL = 30000 + + it('should start and shutdown successfully', (done) => { + // Spawn the bot process using the mock adapter + const hubotPath = path.join(__dirname, '..', 'node_modules', '.bin', 'hubot') + const bot = spawn(hubotPath, ['--adapter', 'mock-adapter', '--name', 'slackbot', '--disable-httpd'], { + cwd: path.join(__dirname, '..'), + env: { + ...process.env, + // Prevent the bot from trying to load environment-specific configs + NODE_ENV: 'test', + // Provide minimal Firebase config to allow startup + FIREBASE_WEB_CONFIG: JSON.stringify({ + apiKey: 'test-api-key', + authDomain: 'test.firebaseapp.com', + projectId: 'test-project', + storageBucket: 'test.appspot.com', + messagingSenderId: '123456789', + appId: 'test-app-id' + }), + // Add PATH to include node_modules/.bin for coffee command + PATH: `${path.join(__dirname, '..', 'node_modules', '.bin')}:${process.env.PATH}`, + // Disable the Heroku keepalive to avoid warning errors + HUBOT_HTTPD: 'false' + } + }) + + let stdout = '' + let stderr = '' + let hasStarted = false + let shutdownTimer = null + + // Collect stdout + bot.stdout.on('data', (data) => { + const output = data.toString() + stdout += output + + // Check for critical errors in stdout (some logging frameworks output to stdout) + if (!hasStarted && (output.includes('Unable to load') || output.includes('is not valid JSON'))) { + // Bot has a critical error, kill it + bot.kill('SIGKILL') + } + }) + + // Collect stderr + bot.stderr.on('data', (data) => { + const output = data.toString() + stderr += output + + // Check if the bot has successfully started + // We look for any deprecation warning or the keepalive error, + // both of which indicate the bot loaded its scripts successfully + if (!hasStarted && (output.includes('DeprecationWarning') || output.includes('hubot-heroku-keepalive'))) { + hasStarted = true + // Give the bot a moment to fully initialize, then gracefully shutdown + shutdownTimer = setTimeout(() => { + bot.kill('SIGTERM') + }, 2000) + } + }) + + // Handle bot exit + bot.on('close', (code) => { + if (shutdownTimer) { + clearTimeout(shutdownTimer) + } + + // Check that the bot started successfully before exiting + expect(hasStarted).toBe(true) + + // Check that there are no critical startup errors in stdout or stderr + // We allow the heroku-keepalive warning and deprecation warnings + const combinedOutput = stdout + stderr + expect(combinedOutput).not.toMatch(/SyntaxError/) + expect(combinedOutput).not.toMatch(/Cannot find module/) + expect(combinedOutput).not.toMatch(/Unable to load/) + expect(combinedOutput).not.toMatch(/is not valid JSON/) + + // Check that the bot exited gracefully (code 0 or null for SIGTERM) + // SIGTERM can result in null or 0 depending on the process + expect(code === 0 || code === null).toBe(true) + + done() + }) + + // Handle errors spawning the process + bot.on('error', (err) => { + if (shutdownTimer) { + clearTimeout(shutdownTimer) + } + fail(`Failed to start bot: ${err.message}`) + done() + }) + + // Safety timeout - if the bot doesn't start within 20 seconds, fail the test + const safetyTimeout = setTimeout(() => { + bot.kill('SIGKILL') + fail('Bot did not start within the timeout period') + done() + }, 20000) + + bot.on('close', () => { + clearTimeout(safetyTimeout) + }) + }) +}) From 841a2caf59a78ece1cef2db79fc065af0aa2a638 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Oct 2025 07:31:46 +0000 Subject: [PATCH 3/6] Address code review feedback - improve error handling and cleanup Co-authored-by: rhamenator <51587867+rhamenator@users.noreply.github.com> --- spec/bot-startup-spec.js | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/spec/bot-startup-spec.js b/spec/bot-startup-spec.js index fdca1f1..8dff914 100644 --- a/spec/bot-startup-spec.js +++ b/spec/bot-startup-spec.js @@ -34,6 +34,13 @@ describe('bot startup', () => { let stderr = '' let hasStarted = false let shutdownTimer = null + let safetyTimeout = null + + // Safety timeout - if the bot doesn't start within 20 seconds, fail the test + safetyTimeout = setTimeout(() => { + bot.kill('SIGTERM') + fail('Bot did not start within the timeout period') + }, 20000) // Collect stdout bot.stdout.on('data', (data) => { @@ -42,8 +49,8 @@ describe('bot startup', () => { // Check for critical errors in stdout (some logging frameworks output to stdout) if (!hasStarted && (output.includes('Unable to load') || output.includes('is not valid JSON'))) { - // Bot has a critical error, kill it - bot.kill('SIGKILL') + // Bot has a critical error, terminate it gracefully + bot.kill('SIGTERM') } }) @@ -69,6 +76,9 @@ describe('bot startup', () => { if (shutdownTimer) { clearTimeout(shutdownTimer) } + if (safetyTimeout) { + clearTimeout(safetyTimeout) + } // Check that the bot started successfully before exiting expect(hasStarted).toBe(true) @@ -93,19 +103,10 @@ describe('bot startup', () => { if (shutdownTimer) { clearTimeout(shutdownTimer) } + if (safetyTimeout) { + clearTimeout(safetyTimeout) + } fail(`Failed to start bot: ${err.message}`) - done() - }) - - // Safety timeout - if the bot doesn't start within 20 seconds, fail the test - const safetyTimeout = setTimeout(() => { - bot.kill('SIGKILL') - fail('Bot did not start within the timeout period') - done() - }, 20000) - - bot.on('close', () => { - clearTimeout(safetyTimeout) }) }) }) From 5ebe6d062598c7d35a05d6377daf8e7f0613dba7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Oct 2025 07:33:53 +0000 Subject: [PATCH 4/6] Fix test completion - call done() after fail() to prevent hanging Co-authored-by: rhamenator <51587867+rhamenator@users.noreply.github.com> --- spec/bot-startup-spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/bot-startup-spec.js b/spec/bot-startup-spec.js index 8dff914..b2695df 100644 --- a/spec/bot-startup-spec.js +++ b/spec/bot-startup-spec.js @@ -40,6 +40,7 @@ describe('bot startup', () => { safetyTimeout = setTimeout(() => { bot.kill('SIGTERM') fail('Bot did not start within the timeout period') + done() }, 20000) // Collect stdout @@ -107,6 +108,7 @@ describe('bot startup', () => { clearTimeout(safetyTimeout) } fail(`Failed to start bot: ${err.message}`) + done() }) }) }) From 05b35512d9c5ef04996e271071d8ae279a3c37b6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Oct 2025 07:36:18 +0000 Subject: [PATCH 5/6] Add guard to prevent done() from being called multiple times Co-authored-by: rhamenator <51587867+rhamenator@users.noreply.github.com> --- spec/bot-startup-spec.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/spec/bot-startup-spec.js b/spec/bot-startup-spec.js index b2695df..ba30282 100644 --- a/spec/bot-startup-spec.js +++ b/spec/bot-startup-spec.js @@ -35,12 +35,21 @@ describe('bot startup', () => { let hasStarted = false let shutdownTimer = null let safetyTimeout = null + let testCompleted = false + + // Helper to complete the test only once + const completeTest = () => { + if (!testCompleted) { + testCompleted = true + done() + } + } // Safety timeout - if the bot doesn't start within 20 seconds, fail the test safetyTimeout = setTimeout(() => { bot.kill('SIGTERM') fail('Bot did not start within the timeout period') - done() + completeTest() }, 20000) // Collect stdout @@ -96,7 +105,7 @@ describe('bot startup', () => { // SIGTERM can result in null or 0 depending on the process expect(code === 0 || code === null).toBe(true) - done() + completeTest() }) // Handle errors spawning the process @@ -108,7 +117,7 @@ describe('bot startup', () => { clearTimeout(safetyTimeout) } fail(`Failed to start bot: ${err.message}`) - done() + completeTest() }) }) }) From a6bb9162c87cf606e949c9f8d133c2492fee6510 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 14 Oct 2025 07:38:24 +0000 Subject: [PATCH 6/6] Centralize cleanup in completeTest function for better reliability Co-authored-by: rhamenator <51587867+rhamenator@users.noreply.github.com> --- spec/bot-startup-spec.js | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/spec/bot-startup-spec.js b/spec/bot-startup-spec.js index ba30282..3aa968b 100644 --- a/spec/bot-startup-spec.js +++ b/spec/bot-startup-spec.js @@ -41,6 +41,13 @@ describe('bot startup', () => { const completeTest = () => { if (!testCompleted) { testCompleted = true + // Clear all timers to ensure proper cleanup + if (shutdownTimer) { + clearTimeout(shutdownTimer) + } + if (safetyTimeout) { + clearTimeout(safetyTimeout) + } done() } } @@ -83,13 +90,6 @@ describe('bot startup', () => { // Handle bot exit bot.on('close', (code) => { - if (shutdownTimer) { - clearTimeout(shutdownTimer) - } - if (safetyTimeout) { - clearTimeout(safetyTimeout) - } - // Check that the bot started successfully before exiting expect(hasStarted).toBe(true) @@ -110,12 +110,6 @@ describe('bot startup', () => { // Handle errors spawning the process bot.on('error', (err) => { - if (shutdownTimer) { - clearTimeout(shutdownTimer) - } - if (safetyTimeout) { - clearTimeout(safetyTimeout) - } fail(`Failed to start bot: ${err.message}`) completeTest() })