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 cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ _Released 03/14/2023 (PENDING)_
**Dependency Updates:**

- Upgraded [`mocha-junit-reporter`](https://github.com/michaelleeallen/mocha-junit-reporter) from `2.1.0` to `2.2.0` to be able to use [new placeholders](https://github.com/michaelleeallen/mocha-junit-reporter/pull/163) such as `[suiteFilename]` or `[suiteName]` when defining the test report name. Addressed in [#25922](https://github.com/cypress-io/cypress/pull/25922).
- Added [`better-sqlite3`]() to support storing test run information which will be sent when recording to the Cloud

## 12.7.0

Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -276,4 +276,4 @@
"sharp": "0.29.3",
"vue-template-compiler": "2.6.12"
}
}
}
18 changes: 11 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,14 @@ 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 Down
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,
}
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
109 changes: 109 additions & 0 deletions packages/server/test/unit/cloud/protocol_spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
import '../../spec_helper'
import ProtocolManager from '../../../lib/cloud/protocol'
import path from 'path'

describe('lib/cloud/protocol', () => {
beforeEach(() => {
process.env.CYPRESS_LOCAL_PROTOCOL_PATH = path.join(__dirname, '..', '..', 'support', 'fixtures', 'cloud', 'protocol', 'test-protocol.js')
})

afterEach(() => {
delete process.env.CYPRESS_LOCAL_PROTOCOL_PATH
})

it('should be able to setup the protocol', async () => {
const protocolManager = new ProtocolManager()

await protocolManager.setupProtocol()

const protocol = (protocolManager as any).protocol

expect(protocolManager.protocolEnabled()).to.be.true
expect(protocol.Debug).not.to.be.undefined
expect(protocol.CDP).not.to.be.undefined
expect(protocol.CY_PROTOCOL_DIR).not.to.be.undefined
expect(protocol.betterSqlite3Binding).not.to.be.undefined
expect(protocol.nodePath).not.to.be.undefined
expect(protocol.Database).not.to.be.undefined
})

it('should be able to connect to the browser', async () => {
const protocolManager = new ProtocolManager()

await protocolManager.setupProtocol()

const protocol = (protocolManager as any).protocol

sinon.stub(protocol, 'connectToBrowser').resolves()

await protocolManager.connectToBrowser({
target: 'target',
host: 'host',
port: 1234,
})

expect(protocol.connectToBrowser).to.be.calledWith({
target: 'target',
host: 'host',
port: 1234,
})
})

it('should be able to initialize a new spec', async () => {
const protocolManager = new ProtocolManager()

await protocolManager.setupProtocol()

const protocol = (protocolManager as any).protocol

sinon.stub(protocol, 'beforeSpec').resolves()

await protocolManager.beforeSpec({
name: 'spec',
relative: 'relative',
absolute: 'absolute',
})

expect(protocol.beforeSpec).to.be.calledWith({
name: 'spec',
relative: 'relative',
absolute: 'absolute',
})
})

it('should be able to initialize a new test', async () => {
const protocolManager = new ProtocolManager()

await protocolManager.setupProtocol()

const protocol = (protocolManager as any).protocol

sinon.stub(protocol, 'beforeTest').resolves()

await protocolManager.beforeTest({
id: 'id',
title: 'test',
wallClockStartedAt: 1234,
})

expect(protocol.beforeTest).to.be.calledWith({
id: 'id',
title: 'test',
wallClockStartedAt: 1234,
})
})

it('should be able to clean up after a spec', async () => {
const protocolManager = new ProtocolManager()

await protocolManager.setupProtocol()

const protocol = (protocolManager as any).protocol

sinon.stub(protocol, 'afterSpec').resolves()

await protocolManager.afterSpec()

expect(protocol.afterSpec).to.be.called
})
})
2 changes: 1 addition & 1 deletion packages/types/src/protocol.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { SpecFile } from '.'
// TODO(protocol): This is basic for now but will evolve as we progress with the protocol work

export interface AppCaptureProtocolInterface {
connectToBrowser (options: { target: string, host: string, port: number }): void
connectToBrowser (options: { target: string, host: string, port: number }): Promise<void>
beforeSpec (spec: SpecFile & { instanceId: string }): void
afterSpec (): void
beforeTest (test: { id: string, title: string, wallClockStartedAt: number }): void
Expand Down
1 change: 1 addition & 0 deletions scripts/binary/binary-cleanup.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ const getDependencyPathsToKeep = async (buildAppDir) => {
'openbsd',
'sunos',
'win32'].map((platform) => path.join(unixBuildAppDir, `node_modules/default-gateway/${platform}.js`)),
path.join(unixBuildAppDir, 'node_modules/vm2/lib/bridge.js'),
])
let esbuildResult
let newEntryPointsFound = true
Expand Down
2 changes: 1 addition & 1 deletion scripts/binary/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ export async function buildCypressApp (options: BuildCypressAppOpts) {
fs.writeJsonSync(meta.distDir('package.json'), {
...packageJsonContents,
scripts: {
postinstall: 'patch-package',
postinstall: 'patch-package && yarn workspace @packages/server build',
},
}, { spaces: 2 })
Copy link
Member

Choose a reason for hiding this comment

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

add comment that this ensure a correct post-install and the native bindings are re-built for the machine per https://github.com/cypress-io/cypress/pull/26168/files#r1144897619


Expand Down
Loading