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 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
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
10 changes: 2 additions & 8 deletions packages/driver/src/cypress.ts
Expand Up @@ -403,15 +403,9 @@ class $Cypress {
break

case 'runner:end':
// mocha runner has finished running the tests
$scriptUtils.destroySourceMaps()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we call $sourceMapUtils.destroySourceMapConsumers() directly, and avoid the indirection of adding this to $scriptUtils?


// 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
6 changes: 5 additions & 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)
.return([script, contents])
Expand Down Expand Up @@ -49,4 +49,8 @@ export default {

return runScriptsFromUrls(specWindow, scripts)
},

destroySourceMaps: () => {
$sourceMapUtils.destroySourceMapConsumers()
},
}
46 changes: 35 additions & 11 deletions packages/driver/src/cypress/source_map_utils.ts
@@ -1,39 +1,53 @@
import _ from 'lodash'
import { SourceMapConsumer } from 'source-map'
import Promise from 'bluebird'
import Bluebird from 'bluebird'

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

import $utils from './utils'
import type BluebirdPromise from 'bluebird'
emilyrohrbough marked this conversation as resolved.
Show resolved Hide resolved

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 = (file, sourceMap) => {
if (!sourceMap) return Promise.resolve(null)
const initializeSourceMapConsumer = (script, sourceMap): BluebirdPromise<BasicSourceMapConsumer | null> => {
if (!sourceMap) return Bluebird.resolve(null)

emilyrohrbough marked this conversation as resolved.
Show resolved Hide resolved
// @ts-ignore
SourceMapConsumer.initialize({
'lib/mappings.wasm': mappingsWasm,
})

return Promise.resolve(new SourceMapConsumer(sourceMap)).then((consumer) => {
sourceMapConsumers[file.fullyQualifiedUrl] = consumer
return Bluebird.resolve(new SourceMapConsumer(sourceMap))
.then((consumer) => {
sourceMapConsumers[script.fullyQualifiedUrl] = consumer

return consumer
})
emilyrohrbough marked this conversation as resolved.
Show resolved Hide resolved
}

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 dataUrlMatch = url.match(regexDataUrl)
const url = _.last(sourceMapMatch) as any

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 @@ -63,14 +77,17 @@ const getSourcePosition = (filePath, position) => {
if (!sourceMapConsumer) return null

const sourcePosition = sourceMapConsumer.originalPositionFor(position)

const { source, line, column } = sourcePosition
emilyrohrbough marked this conversation as resolved.
Show resolved Hide resolved

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-ignore
emilyrohrbough marked this conversation as resolved.
Show resolved Hide resolved
const sourceIndex = sourceMapConsumer._absoluteSources.indexOf(source)
// @ts-ignore
const file = sourceMapConsumer._sources.at(sourceIndex)

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

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

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