Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sending github error status on unhandled errors + Adding max timeout #465

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Sending github error status on unhandled errors + Adding max timeout #465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a824fb
Moving github service initialization outside of code context that inv…
bigbizze Jan 8, 2024
dcf2bd5
Awaiting where needed, adding uncaughtException and unhandledRejectio…
bigbizze Jan 8, 2024
d26edad
Adding maxTimeout config option, which forces us to report an error s…
bigbizze Jan 8, 2024
d272492
Update unit tests
bigbizze Jan 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/app/config/getConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const ciVars = getCIVars(process.env)

const defaultConfig = {
normalizeFilenames: null,
maxTimeout: null,
files: [],
bundlewatchServiceHost: 'https://service.bundlewatch.io', // Can be a custom service, or set to NUll
ci: {
Expand Down
11 changes: 1 addition & 10 deletions src/app/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import getLocalFileDetails from './getLocalFileDetails'
import BundleWatchService from './reporting/BundleWatchService'
import GitHubService from './reporting/GitHubService'
import analyze from './analyze'
import { STATUSES } from './analyze/analyzeFiles'
import getConfig from './config/getConfig'
Expand Down Expand Up @@ -58,16 +57,8 @@ const main = async ({
}
}

const bundlewatchApi = async (customConfig) => {
const bundlewatchApi = async (customConfig, githubService) => {
const config = getConfig(customConfig)
const githubService = new GitHubService({
repoOwner: config.ci.repoOwner,
repoName: config.ci.repoName,
commitSha: config.ci.commitSha,
githubAccessToken: config.ci.githubAccessToken,
})
await githubService.start({ message: 'Checking bundlewatch...' })

try {
const results = await main(config)
if (results.status === STATUSES.FAIL) {
Expand Down
137 changes: 87 additions & 50 deletions src/app/index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,22 @@ import bundlewatchApi from '.'

describe(`bundlewatch Node API`, () => {
it('Works with basic options', async () => {
const result = await bundlewatchApi({
files: [
{
path: './__testdata__/*.jpg',
maxSize: '100kB',
},
],
defaultCompression: 'none',
})
const result = await bundlewatchApi(
{
files: [
{
path: './__testdata__/*.jpg',
maxSize: '100kB',
},
],
defaultCompression: 'none',
},
{
error: () => {},
pass: () => {},
fail: () => {},
},
Comment on lines +20 to +24
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be a bad developer experience if we absolutely need to define error, pass, and fail, here even if they are empty.

I tried to remove them, but then it crashed. I would consider this as a breaking change if we were to roll this out as this is changing the required parameters for BundleWatch API.

Would that make sense to default them to () => {} so we could skip providing them.

)

// TODO: assert logger.warn called

Expand All @@ -24,14 +31,21 @@ describe(`bundlewatch Node API`, () => {
})

it(`Works when files dont exist, shows warning`, async () => {
const result = await bundlewatchApi({
files: [
{
path: './__testdata__/test-file-doesnt-exist.jpg',
maxSize: '100kB',
},
],
})
const result = await bundlewatchApi(
{
files: [
{
path: './__testdata__/test-file-doesnt-exist.jpg',
maxSize: '100kB',
},
],
},
{
error: () => {},
pass: () => {},
fail: () => {},
},
)

delete result.url
expect(result).toMatchSnapshot()
Expand Down Expand Up @@ -64,22 +78,30 @@ describe(`bundlewatch Node API`, () => {

// TODO: assert save was called

const result = await bundlewatchApi({
files: [
{
path: './__testdata__/*.jpg',
maxSize: '1MB',
const result = await bundlewatchApi(
{
files: [
{
path: './__testdata__/*.jpg',
maxSize: '1MB',
},
],
ci: {
githubAccessToken: MOCK_AUTH_TOKEN,
repoOwner: MOCK_REPO.owner,
repoName: MOCK_REPO.name,
repoCurrentBranch: MOCK_REPO.currentBranch,
repoBranchBase: MOCK_REPO.branchBase,
commitSha: MOCK_REPO.branchBase,
},
],
ci: {
githubAccessToken: MOCK_AUTH_TOKEN,
repoOwner: MOCK_REPO.owner,
repoName: MOCK_REPO.name,
repoCurrentBranch: MOCK_REPO.currentBranch,
repoBranchBase: MOCK_REPO.branchBase,
commitSha: MOCK_REPO.branchBase,
},
})

{
error: () => {},
pass: () => {},
fail: () => {},
},
)

delete result.url
expect(result).toMatchSnapshot()
Expand All @@ -88,32 +110,47 @@ describe(`bundlewatch Node API`, () => {
it('Throws validations error when using brotli compression without the package', async () => {
let error
try {
await bundlewatchApi({
files: [
{
path: './__testdata__/*.jpg',
maxSize: '100kB',
},
],
defaultCompression: 'brotli',
})
await bundlewatchApi(
{
files: [
{
path: './__testdata__/*.jpg',
maxSize: '100kB',
},
],
defaultCompression: 'brotli',
},

{
error: () => {},
pass: () => {},
fail: () => {},
},
)
} catch (e) {
error = e
}
expect(error).toMatchSnapshot()
})

it('Normalizes hash when given a normalizeFilenames option', async () => {
const result = await bundlewatchApi({
files: [
{
path: './__testdata__/*.js',
maxSize: '100kB',
},
],
defaultCompression: 'none',
normalizeFilenames: '^.+?(\\.\\w+)\\.(?:js|css)$',
})
const result = await bundlewatchApi(
{
files: [
{
path: './__testdata__/*.js',
maxSize: '100kB',
},
],
defaultCompression: 'none',
normalizeFilenames: '^.+?(\\.\\w+)\\.(?:js|css)$',
},
{
error: () => {},
pass: () => {},
fail: () => {},
},
)

delete result.url
expect(result.fullResults[0].filePath).toMatchInlineSnapshot(
Expand Down
5 changes: 5 additions & 0 deletions src/app/reporting/GitHubService/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ const getContextForFilePath = (filePath) => {
}

class GitHubService {
hasReported = false

constructor({ repoOwner, repoName, commitSha, githubAccessToken }) {
this.repoOwner = repoOwner
this.repoName = repoName
Expand Down Expand Up @@ -93,14 +95,17 @@ class GitHubService {
}

pass({ message, url }) {
this.hasReported = true
return this.update(message, url, 'success')
}

fail({ message, url, filePath }) {
this.hasReported = true
return this.update(message, url, 'failure', filePath)
}

error({ message }) {
this.hasReported = true
return this.update(message, undefined, 'error')
}
}
Expand Down
1 change: 1 addition & 0 deletions src/bin/determineConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ const determineConfig = (cliOptions) => {
files,
defaultCompression: cliOptions.compression || 'gzip',
normalizeFilenames: cliOptions.normalize,
maxTimeout: cliOptions.maxTimeout || 1000 * 60 * 15, // default of 15 minutes
}
}

Expand Down
49 changes: 45 additions & 4 deletions src/bin/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import chalk from 'chalk'
import determineConfig from './determineConfig'
import logger from '../logger'
import bundlewatchApi, { STATUSES } from '../app'
import getConfig from '../app/config/getConfig'
import GitHubService from '../app/reporting/GitHubService'

const prettyPrintResults = (fullResults) => {
logger.log('')
Expand Down Expand Up @@ -36,11 +38,10 @@ const prettyPrintResults = (fullResults) => {
logger.log('')
}

const main = async () => {
const main = async (githubService) => {
const config = determineConfig(program)

if (config.files && config.files.length > 0) {
const results = await bundlewatchApi(config)
const results = await bundlewatchApi(config, githubService)

if (results.url) {
logger.log('')
Expand Down Expand Up @@ -79,8 +80,42 @@ const main = async () => {
}

const mainSafe = async () => {
const config = getConfig(determineConfig(program))
const githubService = new GitHubService({
repoOwner: config.ci.repoOwner,
repoName: config.ci.repoName,
commitSha: config.ci.commitSha,
githubAccessToken: config.ci.githubAccessToken,
})
try {
const errorCode = await main()
await githubService.start({ message: 'Checking bundlewatch...' })
} catch (e) {
await githubService.error({
message: 'Uncaught exception when running bundlewatch',
})
}
process.on('unhandledRejection', async () => {
await githubService.error({
message: 'Uncaught exception when running bundlewatch',
})
})
process.on('uncaughtException', async () => {
await githubService.error({
message: 'Uncaught exception when running bundlewatch',
})
})
try {
const errorCode = await Promise.race([
main(githubService),
new Promise((resolve) => {
if (config.maxTimeout != null) {
setTimeout(resolve, config.maxTimeout)
}
// hang forever if maxTimeout is set to null
}).then(() => {
throw new Error('Max timeout exceeded')
}),
])
return errorCode
} catch (error) {
if (error.type === 'ValidationError') {
Expand All @@ -90,6 +125,12 @@ const mainSafe = async () => {

logger.fatal(`Uncaught exception`, error)
return 1
} finally {
if (!githubService.hasReported) {
await githubService.error({
message: 'Uncaught exception when running bundlewatch',
})
}
}
}

Expand Down