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: allow users to pass true to videoCompression config and only a… #26810

Merged
merged 23 commits into from
Jun 8, 2023
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
11e723d
chore: allow users to pass true to videoCompression config and only a…
AtofStryker May 18, 2023
5f1e071
run ci
AtofStryker May 19, 2023
3b49bd3
Merge branch 'develop' of github.com:cypress-io/cypress into chore/up…
AtofStryker Jun 7, 2023
2826f16
chore: allow zero to be option for CRF as we will coerve it to false …
AtofStryker Jun 7, 2023
7805081
Update cli/CHANGELOG.md
AtofStryker Jun 7, 2023
0b9100c
chore: update videoCompression types
AtofStryker Jun 7, 2023
476b4d2
Merge branch 'chore/update_video_compression_defaults' of github.com:…
AtofStryker Jun 7, 2023
99a8cf2
chore: update changelog
AtofStryker Jun 7, 2023
d919367
Update cli/CHANGELOG.md
AtofStryker Jun 7, 2023
d6cc771
Update cli/types/cypress.d.ts
AtofStryker Jun 7, 2023
7341310
Update packages/config/src/validation.ts
AtofStryker Jun 7, 2023
6943671
chore: update config snapshots
AtofStryker Jun 7, 2023
fb38288
Update packages/config/test/validation.spec.ts
AtofStryker Jun 7, 2023
b653921
chore: add system test on videoCompression=true coersion
AtofStryker Jun 8, 2023
c0fe652
Merge branch 'chore/update_video_compression_defaults' of github.com:…
AtofStryker Jun 8, 2023
53a07c9
chore: document 0 as being false and not a valid CRF option for cypre…
AtofStryker Jun 8, 2023
554177a
Merge branch 'develop' into chore/update_video_compression_defaults
AtofStryker Jun 8, 2023
0292aa5
chore: fix types
AtofStryker Jun 8, 2023
09bad26
Merge branch 'chore/update_video_compression_defaults' of github.com:…
AtofStryker Jun 8, 2023
7b3bde9
chore: fix types
AtofStryker Jun 8, 2023
bd7e247
Merge branch 'develop' of github.com:cypress-io/cypress into chore/up…
AtofStryker Jun 8, 2023
2eb4885
chore: fix change to log as feature and not bugfix
AtofStryker Jun 8, 2023
e86dcb1
chore: fix server side unit tests
AtofStryker Jun 8, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,13 @@
<!-- See the ../guides/writing-the-cypress-changelog.md for details on writing the changelog. -->

## 12.14.1

_Released 06/20/2023 (PENDING)_

**Bugfixes:**

- The [`videoCompression`](https://docs.cypress.io/guides/references/configuration#Videos) configuration option now accepts both a boolean or a Constant Rate Factor (CRF) number between `0` and `51`. The `videoCompression` default value is still `32` CRF and when `videoCompression` is set to `true` the default of `32` CRF will be used. Addresses [#26658](https://github.com/cypress-io/cypress/issues/26658).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- The [`videoCompression`](https://docs.cypress.io/guides/references/configuration#Videos) configuration option now accepts both a boolean or a Constant Rate Factor (CRF) number between `0` and `51`. The `videoCompression` default value is still `32` CRF and when `videoCompression` is set to `true` the default of `32` CRF will be used. Addresses [#26658](https://github.com/cypress-io/cypress/issues/26658).
- The [`videoCompression`](https://docs.cypress.io/guides/references/configuration#Videos) configuration option now accepts both a boolean or a Constant Rate Factor (CRF) number between `1` and `51`. The `videoCompression` default value is still `32` CRF and when `videoCompression` is set to `true` the default of `32` CRF will be used. Addresses [#26658](https://github.com/cypress-io/cypress/issues/26658).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 is still a valid number for CRF, we just don't compress the video since a CRF of 0 is essentially a non compressed video

Copy link
Member

Choose a reason for hiding this comment

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

I think its better to be explicit that this disabled compression even if it is a valid CRF value. If I didn't have context, I'd expect 0 to compress since false is called out to prevent compression.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got this added in 53a07c9

AtofStryker marked this conversation as resolved.
Show resolved Hide resolved

## 12.14.0

_Released 06/07/2023_
Expand Down
7 changes: 5 additions & 2 deletions cli/types/cypress.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3004,10 +3004,13 @@ declare namespace Cypress {
*/
trashAssetsBeforeRuns: boolean
/**
* The quality setting for the video compression, in Constant Rate Factor (CRF). The value can be false to disable compression or a value between 0 and 51, where a lower value results in better quality (at the expense of a higher file size).
* The quality setting for the video compression, in Constant Rate Factor (CRF).
* The value can be false to disable compression.
* The value of true can use the default CRF of 32.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The value can be false to disable compression.
* The value of true can use the default CRF of 32.
* Enable compression by passing true to use the default CRF of 32.

* A number between 0 and 51, where a lower value results in better quality (at the expense of a higher file size).
Copy link
Member

@emilyrohrbough emilyrohrbough Jun 7, 2023

Choose a reason for hiding this comment

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

Suggested change
* A number between 0 and 51, where a lower value results in better quality (at the expense of a higher file size).
* Compress at custom CRF by passing a number between 1 and 51, where a lower value results in better quality (at the expense of a higher file size).
* Disable compression by passing false or 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

0 is still a valid number for CRF, we just don't compress the video since a CRF of 0 is essentially a non compressed video

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we are using 0 to skip compression and not break users, I got the updates in on 53a07c9

* @default 32
*/
videoCompression: number | false
videoCompression: number | boolean
/**
* Whether Cypress will record a video of the test run when running headlessly.
* @default true
Expand Down
260 changes: 136 additions & 124 deletions packages/config/__snapshots__/validation.spec.ts.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
exports['missing https protocol'] = {
"key": "clientCertificates[0].url",
"value": "http://url.com",
"type": "an https protocol"
'key': 'clientCertificates[0].url',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the husky hook did it I swear 😅

'value': 'http://url.com',
'type': 'an https protocol',
}

exports['invalid url'] = {
"key": "clientCertificates[0].url",
"value": "not *",
"type": "a valid URL"
'key': 'clientCertificates[0].url',
'value': 'not *',
'type': 'a valid URL',
}

exports['undefined browsers'] = `
Expand All @@ -19,208 +19,220 @@ Expected at least one browser
`

exports['browsers list with a string'] = {
"key": "name",
"value": "foo",
"type": "a non-empty string",
"list": "browsers"
'key': 'name',
'value': 'foo',
'type': 'a non-empty string',
'list': 'browsers',
}

exports['invalid retry value'] = {
"key": "mockConfigKey",
"value": "1",
"type": "a positive number or null or an object with keys \"openMode\" and \"runMode\" with values of numbers or nulls"
'key': 'mockConfigKey',
'value': '1',
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers or nulls',
}

exports['invalid retry object'] = {
"key": "mockConfigKey",
"value": {
"fakeMode": 1
'key': 'mockConfigKey',
'value': {
'fakeMode': 1,
},
"type": "a positive number or null or an object with keys \"openMode\" and \"runMode\" with values of numbers or nulls"
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers or nulls',
}

exports['not qualified url'] = {
"key": "mockConfigKey",
"value": "url.com",
"type": "a fully qualified URL (starting with `http://` or `https://`)"
'key': 'mockConfigKey',
'value': 'url.com',
'type': 'a fully qualified URL (starting with `http://` or `https://`)',
}

exports['empty string'] = {
"key": "mockConfigKey",
"value": "",
"type": "a fully qualified URL (starting with `http://` or `https://`)"
'key': 'mockConfigKey',
'value': '',
'type': 'a fully qualified URL (starting with `http://` or `https://`)',
}

exports['not string or array'] = {
"key": "mockConfigKey",
"value": null,
"type": "a string or an array of strings"
'key': 'mockConfigKey',
'value': null,
'type': 'a string or an array of strings',
}

exports['array of non-strings'] = {
"key": "mockConfigKey",
"value": [
'key': 'mockConfigKey',
'value': [
1,
2,
3
3,
],
"type": "a string or an array of strings"
'type': 'a string or an array of strings',
}

exports['not one of the strings error message'] = {
"key": "test",
"value": "nope",
"type": "one of these values: \"foo\", \"bar\""
'key': 'test',
'value': 'nope',
'type': 'one of these values: "foo", "bar"',
}

exports['number instead of string'] = {
"key": "test",
"value": 42,
"type": "one of these values: \"foo\", \"bar\""
'key': 'test',
'value': 42,
'type': 'one of these values: "foo", "bar"',
}

exports['null instead of string'] = {
"key": "test",
"value": null,
"type": "one of these values: \"foo\", \"bar\""
'key': 'test',
'value': null,
'type': 'one of these values: "foo", "bar"',
}

exports['not one of the numbers error message'] = {
"key": "test",
"value": 4,
"type": "one of these values: 1, 2, 3"
'key': 'test',
'value': 4,
'type': 'one of these values: 1, 2, 3',
}

exports['string instead of a number'] = {
"key": "test",
"value": "foo",
"type": "one of these values: 1, 2, 3"
'key': 'test',
'value': 'foo',
'type': 'one of these values: 1, 2, 3',
}

exports['null instead of a number'] = {
"key": "test",
"value": null,
"type": "one of these values: 1, 2, 3"
'key': 'test',
'value': null,
'type': 'one of these values: 1, 2, 3',
}

exports['config/src/validation .isValidClientCertificatesSet returns error message for certs not passed as an array array 1'] = {
"key": "mockConfigKey",
"value": "1",
"type": "a positive number or null or an object with keys \"openMode\" and \"runMode\" with values of numbers or nulls"
'key': 'mockConfigKey',
'value': '1',
'type': 'a positive number or null or an object with keys "openMode" and "runMode" with values of numbers or nulls',
}

exports['config/src/validation .isValidClientCertificatesSet returns error message for certs object without url 1'] = {
"key": "clientCertificates[0].url",
"type": "a URL matcher"
'key': 'clientCertificates[0].url',
'type': 'a URL matcher',
}

exports['config/src/validation .isValidBrowser passes valid browsers and forms error messages for invalid ones isValidBrowser 1'] = {
"name": "isValidBrowser",
"behavior": [
'name': 'isValidBrowser',
'behavior': [
{
"given": {
"name": "Chrome",
"displayName": "Chrome Browser",
"family": "chromium",
"path": "/path/to/chrome",
"version": "1.2.3",
"majorVersion": 1
'given': {
'name': 'Chrome',
'displayName': 'Chrome Browser',
'family': 'chromium',
'path': '/path/to/chrome',
'version': '1.2.3',
'majorVersion': 1,
},
"expect": true
'expect': true,
},
{
"given": {
"name": "FF",
"displayName": "Firefox",
"family": "firefox",
"path": "/path/to/firefox",
"version": "1.2.3",
"majorVersion": "1"
'given': {
'name': 'FF',
'displayName': 'Firefox',
'family': 'firefox',
'path': '/path/to/firefox',
'version': '1.2.3',
'majorVersion': '1',
},
"expect": true
'expect': true,
},
{
"given": {
"name": "Electron",
"displayName": "Electron",
"family": "chromium",
"path": "",
"version": "99.101.3",
"majorVersion": 99
'given': {
'name': 'Electron',
'displayName': 'Electron',
'family': 'chromium',
'path': '',
'version': '99.101.3',
'majorVersion': 99,
},
"expect": true
'expect': true,
},
{
"given": {
"name": "No display name",
"family": "chromium"
'given': {
'name': 'No display name',
'family': 'chromium',
},
"expect": {
"key": "displayName",
"value": {
"name": "No display name",
"family": "chromium"
'expect': {
'key': 'displayName',
'value': {
'name': 'No display name',
'family': 'chromium',
},
"type": "a non-empty string"
}
'type': 'a non-empty string',
},
},
{
"given": {
"name": "bad family",
"displayName": "Bad family browser",
"family": "unknown family"
'given': {
'name': 'bad family',
'displayName': 'Bad family browser',
'family': 'unknown family',
},
"expect": {
"key": "family",
"value": {
"name": "bad family",
"displayName": "Bad family browser",
"family": "unknown family"
'expect': {
'key': 'family',
'value': {
'name': 'bad family',
'displayName': 'Bad family browser',
'family': 'unknown family',
},
"type": "either chromium, firefox or webkit"
}
}
]
'type': 'either chromium, firefox or webkit',
},
},
],
}

exports['config/src/validation .isPlainObject returns error message when value is a not an object 1'] = {
"key": "mockConfigKey",
"value": 1,
"type": "a plain object"
'key': 'mockConfigKey',
'value': 1,
'type': 'a plain object',
}

exports['config/src/validation .isNumber returns error message when value is a not a number 1'] = {
"key": "mockConfigKey",
"value": "string",
"type": "a number"
'key': 'mockConfigKey',
'value': 'string',
'type': 'a number',
}

exports['config/src/validation .isNumberOrFalse returns error message when value is a not number or false 1'] = {
"key": "mockConfigKey",
"value": null,
"type": "a number or false"
'key': 'mockConfigKey',
'value': null,
'type': 'a number or false',
}

exports['config/src/validation .isBoolean returns error message when value is a not a string 1'] = {
"key": "mockConfigKey",
"value": 1,
"type": "a string"
'key': 'mockConfigKey',
'value': 1,
'type': 'a string',
}

exports['config/src/validation .isString returns error message when value is a not a string 1'] = {
"key": "mockConfigKey",
"value": 1,
"type": "a string"
'key': 'mockConfigKey',
'value': 1,
'type': 'a string',
}

exports['config/src/validation .isArray returns error message when value is a non-array 1'] = {
"key": "mockConfigKey",
"value": 1,
"type": "an array"
'key': 'mockConfigKey',
'value': 1,
'type': 'an array',
}

exports['config/src/validation .isStringOrFalse returns error message when value is neither string nor false 1'] = {
"key": "mockConfigKey",
"value": null,
"type": "a string or false"
'key': 'mockConfigKey',
'value': null,
'type': 'a string or false',
}

exports['invalid lower bound'] = {
'key': 'test',
'value': -1,
'type': 'a valid CRF number between 0 & 51, false to disable compression, or true to use the default compression of 32',
}

exports['invalid upper bound'] = {
'key': 'test',
'value': 52,
'type': 'a valid CRF number between 0 & 51, false to disable compression, or true to use the default compression of 32',
}
2 changes: 1 addition & 1 deletion packages/config/src/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,7 @@ const driverConfigOptions: Array<DriverConfigOption> = [
}, {
name: 'videoCompression',
defaultValue: 32,
validation: validate.isNumberOrFalse,
validation: validate.isValidCrfOrBoolean,
}, {
name: 'videosFolder',
defaultValue: 'cypress/videos',
Expand Down
Loading