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: add better-sqlite3 dependency #26168

Merged
merged 14 commits into from
Mar 23, 2023
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ module.exports = {
rules: {
'no-restricted-properties': 'off',
'no-restricted-syntax': 'off',
'no-console': 'off',
},
},
],
Expand Down
4 changes: 4 additions & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@ _Released 03/28/2023 (PENDING)_
**Misc:**

- Made some minor styling updates to the Debug page. Addresses [#26041](https://github.com/cypress-io/cypress/issues/26041).

**Dependency Updates:**

- Added [`better-sqlite3`](https://github.com/WiseLibs/better-sqlite3) to support storing test run information which will be sent when recording to the Cloud

## 12.8.1

Expand Down
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@
"ensure-deps": "./scripts/ensure-dependencies.sh",
"get-next-version": "node scripts/get-next-version.js",
"postinstall": "node ./scripts/run-postInstall.js",
"lint": "lerna run lint --no-bail --concurrency 2",
"lint": "lerna run lint --no-bail --concurrency 2 && eslint --ext .js,.ts,.json, scripts",
"prepare-release-artifacts": "node ./scripts/prepare-release-artifacts.js",
"npm-release": "node scripts/npm-release.js",
"prestart": "yarn ensure-deps",
Expand Down Expand Up @@ -276,4 +276,4 @@
"sharp": "0.29.3",
"vue-template-compiler": "2.6.12"
}
}
}
20 changes: 17 additions & 3 deletions packages/server/lib/browsers/browser-cri-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import Debug from 'debug'
import { _connectAsync, _getDelayMsForRetry } from './protocol'
import * as errors from '../errors'
import { create, CriClient } from './cri-client'
import type { ProtocolManager } from '@packages/types'

const debug = Debug('cypress:server:browsers:browser-cri-client')

Expand Down Expand Up @@ -91,7 +92,7 @@ const retryWithIncreasingDelay = async <T>(retryable: () => Promise<T>, browserN

export class BrowserCriClient {
currentlyAttachedTarget: CriClient | undefined
private constructor (private browserClient: CriClient, private versionInfo, public host: string, public port: number, private browserName: string, private onAsynchronousError: Function) {}
private constructor (private browserClient: CriClient, private versionInfo, public host: string, public port: number, private browserName: string, private onAsynchronousError: Function, private protocolManager?: ProtocolManager) {}

/**
* Factory method for the browser cri client. Connects to the browser and then returns a chrome remote interface wrapper around the
Expand All @@ -103,14 +104,14 @@ export class BrowserCriClient {
* @param onAsynchronousError callback for any cdp fatal errors
* @returns a wrapper around the chrome remote interface that is connected to the browser target
*/
static async create (hosts: string[], port: number, browserName: string, onAsynchronousError: Function, onReconnect?: (client: CriClient) => void): Promise<BrowserCriClient> {
static async create (hosts: string[], port: number, browserName: string, onAsynchronousError: Function, onReconnect?: (client: CriClient) => void, protocolManager?: ProtocolManager): Promise<BrowserCriClient> {
const host = await ensureLiveBrowser(hosts, port, browserName)

return retryWithIncreasingDelay(async () => {
const versionInfo = await CRI.Version({ host, port, useHostName: true })
const browserClient = await create(versionInfo.webSocketDebuggerUrl, onAsynchronousError, undefined, undefined, onReconnect)

return new BrowserCriClient(browserClient, versionInfo, host!, port, browserName, onAsynchronousError)
return new BrowserCriClient(browserClient, versionInfo, host!, port, browserName, onAsynchronousError, protocolManager)
}, browserName, port)
}

Expand Down Expand Up @@ -149,6 +150,12 @@ export class BrowserCriClient {
throw new Error(`Could not find url target in browser ${url}. Targets were ${JSON.stringify(targets)}`)
}

await this.protocolManager?.connectToBrowser({
target: target.targetId,
host: this.host,
port: this.port,
})

this.currentlyAttachedTarget = await create(target.targetId, this.onAsynchronousError, this.host, this.port)

return this.currentlyAttachedTarget
Expand Down Expand Up @@ -181,6 +188,12 @@ export class BrowserCriClient {
])

if (target) {
await this.protocolManager?.connectToBrowser({
target: target.targetId,
host: this.host,
port: this.port,
})

this.currentlyAttachedTarget = await create(target.targetId, this.onAsynchronousError, this.host, this.port)
}
}
Expand All @@ -189,6 +202,7 @@ export class BrowserCriClient {
* Closes the browser client socket as well as the socket for the currently attached page target
*/
close = async () => {
this.protocolManager?.close()
if (this.currentlyAttachedTarget) {
await this.currentlyAttachedTarget.close()
}
Expand Down
15 changes: 4 additions & 11 deletions packages/server/lib/browsers/chrome.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import type { Browser, BrowserInstance } from './types'
import { BrowserCriClient } from './browser-cri-client'
import type { CriClient } from './cri-client'
import type { Automation } from '../automation'
import type { BrowserLaunchOpts, BrowserNewTabOpts, RunModeVideoApi } from '@packages/types'
import type { BrowserLaunchOpts, BrowserNewTabOpts, ProtocolManager, RunModeVideoApi } from '@packages/types'
import memory from './memory'

const debug = debugModule('cypress:server:browsers:chrome')
Expand Down Expand Up @@ -569,9 +569,8 @@ export = {
/**
* Clear instance state for the chrome instance, this is normally called in on kill or on exit.
*/
clearInstanceState () {
clearInstanceState (protocolManager?: ProtocolManager) {
debug('closing remote interface client')

// Do nothing on failure here since we're shutting down anyway
browserCriClient?.close().catch()
browserCriClient = undefined
Expand Down Expand Up @@ -640,12 +639,6 @@ export = {
this._handleDownloads(pageCriClient, options.downloadsFolder, automation),
])

await options.protocolManager?.connectToBrowser({
target: pageCriClient.targetId,
host: browserCriClient.host,
port: browserCriClient.port,
})

await this._navigateUsingCRI(pageCriClient, url)

await this._handlePausedRequests(pageCriClient)
Expand Down Expand Up @@ -715,7 +708,7 @@ export = {
// navigate to the actual url
if (!options.onError) throw new Error('Missing onError in chrome#open')

browserCriClient = await BrowserCriClient.create(['127.0.0.1'], port, browser.displayName, options.onError, onReconnect)
browserCriClient = await BrowserCriClient.create(['127.0.0.1'], port, browser.displayName, options.onError, onReconnect, options.protocolManager)

la(browserCriClient, 'expected Chrome remote interface reference', browserCriClient)

Expand All @@ -734,7 +727,7 @@ export = {
launchedBrowser.browserCriClient = browserCriClient

launchedBrowser.kill = (...args) => {
this.clearInstanceState()
this.clearInstanceState(options.protocolManager)

debug('closing chrome')

Expand Down
24 changes: 17 additions & 7 deletions packages/server/lib/cloud/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ import { NodeVM } from 'vm2'
import Debug from 'debug'
import CDP from 'chrome-remote-interface'
import type { ProtocolManager, AppCaptureProtocolInterface } from '@packages/types'

// TODO(protocol): This is basic for now but will evolve as we progress with the protocol wor
import Database from 'better-sqlite3'
import path from 'path'
import os from 'os'

const debug = Debug('cypress:server:protocol')

Expand All @@ -19,9 +20,13 @@ const setupProtocol = async (url?: string): Promise<AppCaptureProtocolInterface
}

if (script) {
const cypressProtocolDirectory = path.join(os.tmpdir(), 'cypress', 'protocol')

// TODO(protocol): Handle any errors here appropriately. Likely, we will want to handle all errors in the initialization process similarly (e.g. downloading, file permissions, etc.)
await fs.ensureDir(cypressProtocolDirectory)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like I've seen file permissions issues before (maybe just docker? worth adding a try-catch to safely bail if we fail here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this path will have several errors associated with it that we will be handling later (it's being caught one level higher right now). I'll add a TODO here to make sure this gets addressed when we dig into error handling for initializing the protocol.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed here: 74784cd (#26168)

const vm = new NodeVM({
console: 'inherit',
sandbox: { Debug, CDP },
sandbox: { nodePath: path, Debug, CDP, Database, CY_PROTOCOL_DIR: cypressProtocolDirectory, betterSqlite3Binding: path.join(require.resolve('better-sqlite3/build/Release/better_sqlite3.node')) },
})

const { AppCaptureProtocol } = vm.run(script)
Expand All @@ -34,6 +39,7 @@ const setupProtocol = async (url?: string): Promise<AppCaptureProtocolInterface

class ProtocolManagerImpl implements ProtocolManager {
private protocol: AppCaptureProtocolInterface | undefined
private db: Database

protocolEnabled (): boolean {
return !!this.protocol
Expand All @@ -45,16 +51,15 @@ class ProtocolManagerImpl implements ProtocolManager {
this.protocol = await setupProtocol(url)
}

connectToBrowser (options) {
async connectToBrowser (options) {
debug('connecting to browser for new spec')
this.protocol?.connectToBrowser(options)

return this.protocol?.connectToBrowser(options)
}

beforeSpec (spec) {
debug('initializing new spec %O', spec)
this.protocol?.beforeSpec(spec)

// Initialize DB here
}

afterSpec () {
Expand All @@ -66,6 +71,11 @@ class ProtocolManagerImpl implements ProtocolManager {
debug('initialize new test %O', test)
this.protocol?.beforeTest(test)
}

close () {
debug('closing')
this.protocol?.close()
}
}

export default ProtocolManagerImpl
3 changes: 3 additions & 0 deletions packages/server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"private": true,
"main": "index.js",
"scripts": {
"build": "electron-rebuild -o better-sqlite3",
Copy link
Member

@emilyrohrbough emilyrohrbough Mar 22, 2023

Choose a reason for hiding this comment

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

How come you didn't update the build-prod script to call yarn build? It seems this would remove the need to update the scripts/binary/build.ts script. Is there a reason to do it as a post-install when building the production binary? If there is a reason, can we add a comment in the scripts/binary/build.ts file?

Copy link
Collaborator Author

@ryanthemanuel ryanthemanuel Mar 22, 2023

Choose a reason for hiding this comment

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

I tried what you suggested first and unfortunately that didn't work. In the binary build we actually do a fresh yarn install with a very minimal postinstall step that does not run build or build-prod so anything we would have done in build-prod earlier would be overridden at that point.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It seems like the rule might be: anything we do to the node_modules dependencies themselves needs to be done here. This includes patching packages and rebuilding native bindings.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏻 Makes sense. I assume you've found that via trial and error 😅

"build-prod": "tsc || echo 'built, with type errors'",
"check-ts": "tsc --noEmit && yarn -s tslint",
"clean-deps": "rimraf node_modules",
Expand Down Expand Up @@ -37,6 +38,7 @@
"ansi_up": "5.0.0",
"ast-types": "0.13.3",
"base64url": "^3.0.1",
"better-sqlite3": "8.2.0",
Copy link
Member

Choose a reason for hiding this comment

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

typically we document dependency bumps. Should this have a changelog entry to mention we added this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed here: 74784cd (#26168)

"black-hole-stream": "0.0.1",
"bluebird": "3.7.2",
"bundle-require": "3.0.4",
Expand Down Expand Up @@ -138,6 +140,7 @@
"@cypress/json-schemas": "5.39.0",
"@cypress/sinon-chai": "2.9.1",
"@cypress/webpack-dev-server": "0.0.0-development",
"@electron/rebuild": "3.2.10",
"@ffprobe-installer/ffprobe": "1.1.0",
"@packages/config": "0.0.0-development",
"@packages/data-context": "0.0.0-development",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/* global Debug, CDP, CY_PROTOCOL_DIR, betterSqlite3Binding, nodePath, Database */

const AppCaptureProtocol = class {
constructor () {
this.Debug = Debug
this.CDP = CDP
this.CY_PROTOCOL_DIR = CY_PROTOCOL_DIR
this.betterSqlite3Binding = betterSqlite3Binding
this.nodePath = nodePath
this.Database = Database

this.connectToBrowser = this.connectToBrowser.bind(this)
this.beforeSpec = this.beforeSpec.bind(this)
this.afterSpec = this.afterSpec.bind(this)
this.beforeTest = this.beforeTest.bind(this)
}

async connectToBrowser ({
target,
host,
port,
}) {
return Promise.resolve()
}

beforeSpec (spec) {

}

afterSpec (spec) {
}

beforeTest (test) {

}
}

module.exports = {
AppCaptureProtocol,
}
26 changes: 24 additions & 2 deletions packages/server/test/unit/browsers/browser-cri-client_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { expect, proxyquire, sinon } from '../../spec_helper'
import * as protocol from '../../../lib/browsers/protocol'
import { stripAnsi } from '@packages/errors'
import net from 'net'
import { ProtocolManager } from '@packages/types'

const HOST = '127.0.0.1'
const PORT = 50505
Expand All @@ -22,7 +23,7 @@ describe('lib/browsers/cri-client', function () {
Version: sinon.SinonStub
}
let onError: sinon.SinonStub
let getClient: () => ReturnType<typeof BrowserCriClient.create>
let getClient: (protocolManager?: ProtocolManager) => ReturnType<typeof BrowserCriClient.create>

beforeEach(function () {
sinon.stub(protocol, '_connectAsync')
Expand All @@ -47,7 +48,7 @@ describe('lib/browsers/cri-client', function () {
'chrome-remote-interface': criImport,
})

getClient = () => browserCriClient.BrowserCriClient.create(['127.0.0.1'], PORT, 'Chrome', onError)
getClient = (protocolManager) => browserCriClient.BrowserCriClient.create(['127.0.0.1'], PORT, 'Chrome', onError, undefined, protocolManager)
})

context('.create', function () {
Expand Down Expand Up @@ -151,6 +152,27 @@ describe('lib/browsers/cri-client', function () {
expect(client).to.be.equal(mockPageClient)
})

it('creates a page client when the passed in url is found and notifies the protocol manager', async function () {
const mockPageClient = {}
const protocolManager: any = {
connectToBrowser: sinon.stub().resolves(),
}

send.withArgs('Target.getTargets').resolves({ targetInfos: [{ targetId: '1', url: 'http://foo.com' }, { targetId: '2', url: 'http://bar.com' }] })
criClientCreateStub.withArgs('1', onError, HOST, PORT).resolves(mockPageClient)

const browserClient = await getClient(protocolManager)

const client = await browserClient.attachToTargetUrl('http://foo.com')

expect(client).to.be.equal(mockPageClient)
expect(protocolManager.connectToBrowser).to.be.calledWith({
host: HOST,
port: PORT,
target: '1',
})
})

it('retries when the passed in url is not found', async function () {
sinon.stub(protocol, '_getDelayMsForRetry')
.onFirstCall().returns(100)
Expand Down
10 changes: 0 additions & 10 deletions packages/server/test/unit/browsers/chrome_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -558,10 +558,6 @@ describe('lib/browsers/chrome', () => {
kill: sinon.stub().returns(),
}

const protocolManager = {
connectToBrowser: sinon.stub().resolves(),
}

let onInitializeNewBrowserTabCalled = false
const options = {
...openOpts,
Expand All @@ -572,7 +568,6 @@ describe('lib/browsers/chrome', () => {
onInitializeNewBrowserTab: () => {
onInitializeNewBrowserTabCalled = true
},
protocolManager,
}

sinon.stub(chrome, '_getBrowserCriClient').returns(browserCriClient)
Expand All @@ -588,11 +583,6 @@ describe('lib/browsers/chrome', () => {
expect(chrome._navigateUsingCRI).to.be.called
expect(chrome._handleDownloads).to.be.called
expect(onInitializeNewBrowserTabCalled).to.be.true
expect(protocolManager.connectToBrowser).to.be.calledWith({
host: 'http://localhost',
port: 1234,
target: '1234',
})
})
})

Expand Down
2 changes: 1 addition & 1 deletion packages/server/test/unit/cloud/environment_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { exec } from 'child_process'
import originalResolvePackagePath from 'resolve-package-path'
import proxyquire from 'proxyquire'

describe('lib/cloud/api', () => {
describe('lib/cloud/environment', () => {
beforeEach(() => {
delete process.env.CYPRESS_API_URL
process.env.CYPRESS_ENV_DEPENDENCIES = base64url.encode(JSON.stringify({
Expand Down
Loading