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.

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

feat: removed filter-console dependency and fallback if process.env is not available #624

Merged
merged 4 commits into from
May 24, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion disable-error-filtering.js
Original file line number Diff line number Diff line change
@@ -1 +1,8 @@
process.env.RHTL_DISABLE_ERROR_FILTERING = true
try {
process.env.RHTL_DISABLE_ERROR_FILTERING = true
} catch {
// falling back in the case that process.env.RHTL_DISABLE_ERROR_FILTERING cannot be accessed (e.g. browser environment)
console.warn(
'Could not disable error filtering as process.env.RHTL_DISABLE_ERROR_FILTERING could not be accessed.'
)
}
9 changes: 8 additions & 1 deletion dont-cleanup-after-each.js
Original file line number Diff line number Diff line change
@@ -1 +1,8 @@
process.env.RHTL_SKIP_AUTO_CLEANUP = true
try {
process.env.RHTL_SKIP_AUTO_CLEANUP = true
} catch {
// falling back in the case that process.env.RHTL_SKIP_AUTO_CLEANUP cannot be accessed (e.g. browser environment)
console.warn(
'Could not skip auto cleanup as process.env.RHTL_SKIP_AUTO_CLEANUP could not be accessed.'
)
}
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
"@types/react": ">=16.9.0",
"@types/react-dom": ">=16.9.0",
"@types/react-test-renderer": ">=16.9.0",
"filter-console": "^0.1.1",
"react-error-boundary": "^3.1.0"
},
"devDependencies": {
Expand Down
11 changes: 10 additions & 1 deletion src/core/cleanup.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,18 @@ function removeCleanup(callback: CleanupCallback) {
cleanupCallbacks = cleanupCallbacks.filter((cb) => cb !== callback)
}

function skipAutoCleanup() {
try {
return !!process.env.RHTL_SKIP_AUTO_CLEANUP
mpeyper marked this conversation as resolved.
Show resolved Hide resolved
} catch {
// falling back in the case that process.env.RHTL_SKIP_AUTO_CLEANUP cannot be accessed (e.g. browser environment)
return false
}
}

function autoRegisterCleanup() {
// Automatically registers cleanup in supported testing frameworks
if (typeof afterEach === 'function' && !process.env.RHTL_SKIP_AUTO_CLEANUP) {
if (typeof afterEach === 'function' && !skipAutoCleanup()) {
afterEach(async () => {
await cleanup()
})
Expand Down
43 changes: 30 additions & 13 deletions src/core/console.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,41 @@
import filterConsole from 'filter-console'
const consoleFilters = [
/^The above error occurred in the <.*?> component:/, // error boundary output
Copy link
Member Author

Choose a reason for hiding this comment

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

I've slightly loosened what this will match, in case a Wrapper throws instead of the TestComponent

/^Error: Uncaught .+/ // jsdom output
]

function suppressErrorOutput() {
if (process.env.RHTL_DISABLE_ERROR_FILTERING) {
return () => {}
}
Comment on lines -4 to -6
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the breaking change I referred to in the PR description

const originalError = console.error

return filterConsole(
[
/^The above error occurred in the <TestComponent> component:/, // error boundary output
/^Error: Uncaught .+/ // jsdom output
],
{
methods: ['error']
const error = (...args: Parameters<typeof originalError>) => {
const message = typeof args[0] === 'string' ? args[0] : null
if (!message || !consoleFilters.some((filter) => filter.test(message))) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to call it out, filter-console would format the string before testing it against the patter (that's why it had a dependency on util), but I found this wasn't necessary for the cases we are trying to filter. Not doing this may be an issue if how the calls from upstream are made ever change or if we introduce any new cases in the future (although I don't think that is likely).

originalError(...args)
}
)
}

console.error = error

return () => {
console.error = originalError
}
}

function errorFilteringDisabled() {
try {
return !!process.env.RHTL_DISABLE_ERROR_FILTERING
mpeyper marked this conversation as resolved.
Show resolved Hide resolved
} catch {
// falling back in the case that process.env.RHTL_DISABLE_ERROR_FILTERING cannot be accessed (e.g. browser environment)
return false
}
}

function enableErrorOutputSuppression() {
// Automatically registers console error suppression and restoration in supported testing frameworks
if (typeof beforeEach === 'function' && typeof afterEach === 'function') {
if (
typeof beforeEach === 'function' &&
typeof afterEach === 'function' &&
!errorFilteringDisabled()
) {
let restoreConsole!: () => void

beforeEach(() => {
Expand Down
40 changes: 40 additions & 0 deletions src/dom/__tests__/autoCleanup.noProcessEnv.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { useEffect } from 'react'

import { ReactHooksRenderer } from '../../types/react'

// This verifies that if process.env is unavailable
// then we still auto-wire up the afterEach for folks
describe('skip auto cleanup (no process.env) tests', () => {
const originalEnv = process.env
let cleanupCalled = false
let renderHook: ReactHooksRenderer['renderHook']

beforeAll(() => {
process.env = {
...process.env,
get RHTL_SKIP_AUTO_CLEANUP(): string | undefined {
throw new Error('expected')
}
}
Comment on lines +13 to +18
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't remove process.env from the test environment all together (other things started to crash), but I could make it error when trying to access the variables.

renderHook = (require('..') as ReactHooksRenderer).renderHook
})

afterAll(() => {
process.env = originalEnv
})

test('first', () => {
const hookWithCleanup = () => {
useEffect(() => {
return () => {
cleanupCalled = true
}
})
}
renderHook(() => hookWithCleanup())
})

test('second', () => {
expect(cleanupCalled).toBe(true)
})
})
47 changes: 0 additions & 47 deletions src/dom/__tests__/errorHook.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,51 +142,4 @@ describe('error hook tests', () => {
expect(result.error).toBe(undefined)
})
})

describe('error output suppression', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

this moved to a new file

test('should allow console.error to be mocked', async () => {
const consoleError = console.error
console.error = jest.fn()

try {
const { rerender, unmount } = renderHook(
(stage) => {
useEffect(() => {
console.error(`expected in effect`)
return () => {
console.error(`expected in unmount`)
}
}, [])
console.error(`expected in ${stage}`)
},
{
initialProps: 'render'
}
)

act(() => {
console.error('expected in act')
})

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 100))
console.error('expected in async act')
})

rerender('rerender')

unmount()

expect(console.error).toBeCalledWith('expected in render')
expect(console.error).toBeCalledWith('expected in effect')
expect(console.error).toBeCalledWith('expected in act')
expect(console.error).toBeCalledWith('expected in async act')
expect(console.error).toBeCalledWith('expected in rerender')
expect(console.error).toBeCalledWith('expected in unmount')
expect(console.error).toBeCalledTimes(6)
} finally {
console.error = consoleError
}
})
})
})
26 changes: 26 additions & 0 deletions src/dom/__tests__/errorSuppression.noProcessEnv.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
// This verifies that if process.env is unavailable
// then we still auto-wire up the afterEach for folks
describe('error output suppression (no process.env) tests', () => {
const originalEnv = process.env
const originalConsoleError = console.error

beforeAll(() => {
process.env = {
...process.env,
get RHTL_DISABLE_ERROR_FILTERING(): string | undefined {
throw new Error('expected')
}
}
require('..')
})

afterAll(() => {
process.env = originalEnv
})

test('should not patch console.error', () => {
expect(console.error).not.toBe(originalConsoleError)
})
})

export {}
75 changes: 75 additions & 0 deletions src/dom/__tests__/errorSuppression.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
import { useEffect } from 'react'

import { ReactHooksRenderer } from '../../types/react'

describe('error output suppression tests', () => {
test('should not suppress relevant errors', () => {
const consoleError = console.error
console.error = jest.fn()

const { suppressErrorOutput } = require('..') as ReactHooksRenderer

try {
const restoreConsole = suppressErrorOutput()

console.error('expected')
console.error(new Error('expected'))
console.error('expected with args', new Error('expected'))

restoreConsole()

expect(console.error).toBeCalledWith('expected')
expect(console.error).toBeCalledWith(new Error('expected'))
expect(console.error).toBeCalledWith('expected with args', new Error('expected'))
expect(console.error).toBeCalledTimes(3)
} finally {
console.error = consoleError
}
})

test('should allow console.error to be mocked', async () => {
const { renderHook, act } = require('..') as ReactHooksRenderer
const consoleError = console.error
console.error = jest.fn()

try {
const { rerender, unmount } = renderHook(
(stage) => {
useEffect(() => {
console.error(`expected in effect`)
return () => {
console.error(`expected in unmount`)
}
}, [])
console.error(`expected in ${stage}`)
},
{
initialProps: 'render'
}
)

act(() => {
console.error('expected in act')
})

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 100))
console.error('expected in async act')
})

rerender('rerender')

unmount()

expect(console.error).toBeCalledWith('expected in render')
expect(console.error).toBeCalledWith('expected in effect')
expect(console.error).toBeCalledWith('expected in act')
expect(console.error).toBeCalledWith('expected in async act')
expect(console.error).toBeCalledWith('expected in rerender')
expect(console.error).toBeCalledWith('expected in unmount')
expect(console.error).toBeCalledTimes(6)
} finally {
console.error = consoleError
}
})
})
2 changes: 1 addition & 1 deletion src/dom/pure.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import ReactDOM from 'react-dom'
import * as ReactDOM from 'react-dom'
Copy link
Member Author

Choose a reason for hiding this comment

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

This prevents the import relying on @babel/runtime for default import interop.

Copy link

@bdwain bdwain May 23, 2021

Choose a reason for hiding this comment

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

so this works the same either way? interesting. i would expect you to need to add .default where you used ReactDOM before

Copy link
Member Author

Choose a reason for hiding this comment

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

It's interesting because import ReactDOM and import * as ReactDOM are not equivalent as per the ES6 import spec, but most compilers/bundlers treat them interchangeably. Babel does this by inserting the default import interop helper when there is no default export for the module, like in React/ReactDOM's case. By changing the the import * as ..., it's actually more correct for these libraries.

import { act } from 'react-dom/test-utils'

import { RendererProps, RendererOptions } from '../types/react'
Expand Down
40 changes: 40 additions & 0 deletions src/native/__tests__/autoCleanup.noProcessEnv.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
import { useEffect } from 'react'

import { ReactHooksRenderer } from '../../types/react'

// This verifies that if process.env is unavailable
// then we still auto-wire up the afterEach for folks
describe('skip auto cleanup (no process.env) tests', () => {
const originalEnv = process.env
let cleanupCalled = false
let renderHook: ReactHooksRenderer['renderHook']

beforeAll(() => {
process.env = {
...process.env,
get RHTL_SKIP_AUTO_CLEANUP(): string | undefined {
throw new Error('expected')
}
}
renderHook = (require('..') as ReactHooksRenderer).renderHook
})

afterAll(() => {
process.env = originalEnv
})

test('first', () => {
const hookWithCleanup = () => {
useEffect(() => {
return () => {
cleanupCalled = true
}
})
}
renderHook(() => hookWithCleanup())
})

test('second', () => {
expect(cleanupCalled).toBe(true)
})
})
47 changes: 0 additions & 47 deletions src/native/__tests__/errorHook.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,51 +142,4 @@ describe('error hook tests', () => {
expect(result.error).toBe(undefined)
})
})

describe('error output suppression', () => {
test('should allow console.error to be mocked', async () => {
const consoleError = console.error
console.error = jest.fn()

try {
const { rerender, unmount } = renderHook(
(stage) => {
useEffect(() => {
console.error(`expected in effect`)
return () => {
console.error(`expected in unmount`)
}
}, [])
console.error(`expected in ${stage}`)
},
{
initialProps: 'render'
}
)

act(() => {
console.error('expected in act')
})

await act(async () => {
await new Promise((resolve) => setTimeout(resolve, 100))
console.error('expected in async act')
})

rerender('rerender')

unmount()

expect(console.error).toBeCalledWith('expected in render')
expect(console.error).toBeCalledWith('expected in effect')
expect(console.error).toBeCalledWith('expected in act')
expect(console.error).toBeCalledWith('expected in async act')
expect(console.error).toBeCalledWith('expected in rerender')
expect(console.error).toBeCalledWith('expected in unmount')
expect(console.error).toBeCalledTimes(6)
} finally {
console.error = consoleError
}
})
})
})