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

chore: destroy source maps consumers when spec finishes #23708

Merged
merged 15 commits into from Sep 13, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 0 additions & 1 deletion packages/driver/README.md
Expand Up @@ -82,7 +82,6 @@ TODO: this data is accurate but also somewhat out of date.
| mocha:fail | Mocha | Cypress | when mocha runner fires its 'fail' event |
| test:end | Mocha | Cypress | when mocha runner fires its 'test end' event |
| test:results:ready | Runner | Anyone | when we receive the 'test:end' event |
| test:after:hooks | Cypress | Cypress | after all hooks have run for a test |
| test:after:run | Cypress | Anyone | after any code has run for a test |
| mocha:end | Mocha | Cypress | when mocha runner fires its 'end' event |
| after:run | Runner | Anyone | after run has finished |
Expand Down
4 changes: 2 additions & 2 deletions packages/driver/cypress/e2e/cypress/script_utils.cy.js
Expand Up @@ -35,8 +35,8 @@ describe('src/cypress/script_utils', () => {
return $scriptUtils.runScripts(scriptWindow, scripts)
.then(() => {
expect($sourceMapUtils.extractSourceMap).to.be.calledTwice
expect($sourceMapUtils.extractSourceMap).to.be.calledWith(scripts[0], 'the script contents')
expect($sourceMapUtils.extractSourceMap).to.be.calledWith(scripts[1], 'the script contents')
expect($sourceMapUtils.extractSourceMap).to.be.calledWith('the script contents')
expect($sourceMapUtils.extractSourceMap).to.be.calledWith('the script contents')
})
})

Expand Down
10 changes: 5 additions & 5 deletions packages/driver/cypress/e2e/cypress/source_map_utils.cy.js
Expand Up @@ -46,19 +46,19 @@ const file2 = createFile('file2')
describe('driver/src/cypress/source_map_utils', () => {
context('.extractSourceMap', () => {
it('returns null if there is no source map embedded', () => {
const sourceMap = $sourceMapUtils.extractSourceMap(file2, testContent)
const sourceMap = $sourceMapUtils.extractSourceMap(testContent)

expect(sourceMap).to.be.null
})

it('returns null if it is not an inline map', () => {
const sourceMap = $sourceMapUtils.extractSourceMap(file2, `${testContent}\n\/\/# sourceMappingURL=foo.map`)
const sourceMap = $sourceMapUtils.extractSourceMap(`${testContent}\n\/\/# sourceMappingURL=foo.map`)

expect(sourceMap).to.be.null
})

it('returns source map when content has an inline map', () => {
const sourceMap = $sourceMapUtils.extractSourceMap(file1, fileContents)
const sourceMap = $sourceMapUtils.extractSourceMap(fileContents)

expect(sourceMap).to.be.eql(sourceMap)
})
Expand All @@ -71,7 +71,7 @@ describe('driver/src/cypress/source_map_utils', () => {
const timeLimit = 10
const startTime = Date.now()

$sourceMapUtils.extractSourceMap(file1, fileContents)
$sourceMapUtils.extractSourceMap(fileContents)

const endTime = Date.now()

Expand All @@ -81,7 +81,7 @@ describe('driver/src/cypress/source_map_utils', () => {

// https://github.com/cypress-io/cypress/issues/7481
it('does not garble utf-8 characters', () => {
const sourceMap = $sourceMapUtils.extractSourceMap(file1, fileContents)
const sourceMap = $sourceMapUtils.extractSourceMap(fileContents)

expect(sourceMap.sourcesContent[1]).to.include('サイプリスは一番')
})
Expand Down
11 changes: 3 additions & 8 deletions packages/driver/src/cypress.ts
Expand Up @@ -9,6 +9,7 @@ import debugFn from 'debug'

import browserInfo from './cypress/browser'
import $scriptUtils from './cypress/script_utils'
import $sourceMapUtils from './cypress/source_map_utils'

import $Commands from './cypress/commands'
import { $Cy } from './cypress/cy'
Expand Down Expand Up @@ -403,15 +404,9 @@ class $Cypress {
break

case 'runner:end':
// mocha runner has finished running the tests
$sourceMapUtils.destroySourceMapConsumers()

// end may have been caused by an uncaught error
// that happened inside of a hook.
//
// when this happens mocha aborts the entire run
// and does not do the usual cleanup so that means
// we have to fire the test:after:hooks and
// test:after:run events ourselves
// mocha runner has finished running the tests
this.emit('run:end')

this.maybeEmitCypressInCypress('mocha', 'end', args[0])
Expand Down
2 changes: 1 addition & 1 deletion packages/driver/src/cypress/script_utils.ts
Expand Up @@ -14,7 +14,7 @@ const fetchScript = (scriptWindow, script) => {
const extractSourceMap = ([script, contents]) => {
script.fullyQualifiedUrl = `${window.top!.location.origin}${script.relativeUrl}`.replace(/ /g, '%20')

const sourceMap = $sourceMapUtils.extractSourceMap(script, contents)
const sourceMap = $sourceMapUtils.extractSourceMap(contents)

return $sourceMapUtils.initializeSourceMapConsumer(script, sourceMap)
.catch((_err) => {
Expand Down
40 changes: 30 additions & 10 deletions packages/driver/src/cypress/source_map_utils.ts
@@ -1,6 +1,7 @@
import _ from 'lodash'
import { SourceMapConsumer } from 'source-map'

import type { BasicSourceMapConsumer } from 'source-map'
// @ts-ignore
import mappingsWasm from 'source-map/lib/mappings.wasm'

Expand All @@ -9,9 +10,9 @@ import $utils from './utils'
const sourceMapExtractionRegex = /\/\/\s*[@#]\s*sourceMappingURL\s*=\s*(data:[^\s]*)/g
const regexDataUrl = /data:[^;\n]+(?:;charset=[^;\n]+)?;base64,([a-zA-Z0-9+/]+={0,2})/ // matches data urls

let sourceMapConsumers = {}
let sourceMapConsumers: Record<string, BasicSourceMapConsumer> = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use Map, I think it makes the intention a bit more clear.


const initializeSourceMapConsumer = async (file, sourceMap) => {
const initializeSourceMapConsumer = async (script, sourceMap): Promise<BasicSourceMapConsumer | null> => {
if (!sourceMap) return null

// @ts-ignore
Expand All @@ -21,18 +22,29 @@ const initializeSourceMapConsumer = async (file, sourceMap) => {

const consumer = await new SourceMapConsumer(sourceMap)

sourceMapConsumers[file.fullyQualifiedUrl] = consumer
sourceMapConsumers[script.fullyQualifiedUrl] = consumer

return consumer
}

const extractSourceMap = (file, fileContents) => {
let sourceMapMatch = fileContents.match(sourceMapExtractionRegex)
const extractSourceMap = (fileContents) => {
let dataUrlMatch

if (!sourceMapMatch) return null
try {
let sourceMapMatch = fileContents.match(sourceMapExtractionRegex)

if (!sourceMapMatch) return null

const url = _.last(sourceMapMatch) as any

const url = _.last(sourceMapMatch) as any
const dataUrlMatch = url.match(regexDataUrl)
dataUrlMatch = url.match(regexDataUrl)
} catch (err) {
// ignore unable to match regex. there's nothing we
// can do about it and we don't want to thrown an exception
if (err.message === 'Maximum call stack size exceeded') return null
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be worth checking if this error is the same in FF and WebKit

Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest checking the error type instead, but it looks like the error type is inconsistent (InternalError in Firefox; RangeError in Chrome and Safari.), but the error message is the same across browsers.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Errors/Too_much_recursion


throw err
}

if (!dataUrlMatch) return null

Expand Down Expand Up @@ -61,15 +73,16 @@ const getSourcePosition = (filePath, position) => {

if (!sourceMapConsumer) return null

const sourcePosition = sourceMapConsumer.originalPositionFor(position)
const { source, line, column } = sourcePosition
const { source, line, column } = sourceMapConsumer.originalPositionFor(position)

if (!source || line == null || column == null) return

// if the file is outside of the projectRoot
// originalPositionFor will not provide the correct relative path
// https://github.com/cypress-io/cypress/issues/16255
// @ts-expect-error
const sourceIndex = sourceMapConsumer._absoluteSources.indexOf(source)
// @ts-expect-error
const file = sourceMapConsumer._sources.at(sourceIndex)

return {
Expand All @@ -89,9 +102,16 @@ const base64toJs = (base64) => {
}
}

const destroySourceMapConsumers = () => {
Object.values(sourceMapConsumers).forEach((consumer) => {
consumer.destroy()
})
}

export default {
getSourcePosition,
getSourceContents,
extractSourceMap,
initializeSourceMapConsumer,
destroySourceMapConsumers,
}