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.

Sign up for GitHub

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

Jump to bottom

chore: Address skipped specs in server package #22356

Merged
merged 17 commits into from Jun 27, 2022
Merged

chore: Address skipped specs in server package #22356

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
356fbde
chore: Un-skip some tests
astone123 Jun 16, 2022
ee6d2da
Merge branch 'develop' of github.com:cypress-io/cypress into 21966-sk…
astone123 Jun 17, 2022
3239348
chore: Unskip more tests
astone123 Jun 17, 2022
4ae8a31
chore: Use exec rather than spawn
astone123 Jun 17, 2022
4f82a91
chore: Fix test
astone123 Jun 17, 2022
589a82e
chore: Remove events_spec tests
astone123 Jun 21, 2022
eb3666d
Merge branch 'develop' into 21966-skipped-server-specs
astone123 Jun 21, 2022
50cc582
chore: Add defaultValue for specPattern option
astone123 Jun 22, 2022
cafd029
chore: Remove default value
astone123 Jun 23, 2022
25085da
chore: Use os to determine system arch and platform during testing
astone123 Jun 23, 2022
6d41946
Merge branch 'develop' into 21966-skipped-server-specs
astone123 Jun 23, 2022
31a837d
Merge branch 'develop' of github.com:cypress-io/cypress into 21966-sk…
astone123 Jun 24, 2022
e0f17a4
Merge branch '21966-skipped-server-specs' of github.com:cypress-io/cy…
astone123 Jun 24, 2022
3019d1f
chore: Add comment for specPattern defaultValue
astone123 Jun 24, 2022
f32c6cd
chore: Remove comment
astone123 Jun 24, 2022
93ad270
Merge branch 'develop' into 21966-skipped-server-specs
astone123 Jun 24, 2022
b6d1df2
Merge branch 'develop' into 21966-skipped-server-specs
astone123 Jun 24, 2022
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: 1 addition & 0 deletions packages/config/__snapshots__/index.spec.ts.js
View file
Open in desktop
Expand Up @@ -226,6 +226,7 @@ exports['config/src/index .getPublicConfigKeys returns list of public config key
"viewportWidth",
"waitForAnimations",
"watchForFileChanges",
"specPattern",
"browsers",
"hosts",
"isInteractive",
Expand Down
6 changes: 6 additions & 0 deletions packages/config/src/options.ts
View file
Open in desktop
Expand Up @@ -419,6 +419,12 @@ const resolvedOptions: Array<ResolvedConfigOption> = [
canUpdateDuringTestTime: false,
requireRestartOnChange: 'server',
},
// Possibly add a defaultValue for specPattern https://github.com/cypress-io/cypress/issues/22507
{
name: 'specPattern',
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good catch! We seem to also declare the default values in other cases. Looks like we can use the same options.testingType ternary that's being used for viewportWidth etc to populate the defaults that are declared in this file already in the defaultSpecPattern object. What do you think?

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 added that but then I just removed it again... I'm confused about what defaultValue does here. It looks like it was messing up some of the validation in the tests when I had it in there 😕

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make an issue and add a comment for this then, totally worth investigate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an issue and a comment here

validation: validate.isStringOrArrayOfStrings,
canUpdateDuringTestTime: false,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something, this seems the opposite of what we want:

options.testingType === 'component' ? defaultSpecPattern.e2e

If we are using component testing, default to the e2e spec pattern?

I think you want these:

cypress/packages/config/src/options.ts

Lines 109 to 112 in 50cc582

export const defaultSpecPattern = {
e2e: 'cypress/e2e/**/*.cy.{js,jsx,ts,tsx}',
component: '**/*.cy.{js,jsx,ts,tsx}',
}

Maybe it should be

defaultValue: (options: Record<string, any> = {}) =>defaultSpecPattern[options.testingType],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that was wrong... I ended up removing the defaultValue because it was causing issues. It seems like options.testingType was undefined sometimes when inside of the defaultValue function. I don't think that we need that property here based on the tests

]

const runtimeOptions: Array<RuntimeConfigOption> = [
Expand Down
4 changes: 2 additions & 2 deletions packages/server/test/integration/cli_spec.js
View file
Open in desktop
Expand Up @@ -44,8 +44,8 @@ describe('CLI Interface', () => {
})
})

// TODO:(tgriesser) originally skipped this, not sure why
it.skip('writes out package.json and exits', (done) => {
// This fails on MacOS due to an apparent limit on the buffer size of stdout
it('writes out package.json and exits', (done) => {
return cp.exec('npm run dev -- --return-pkg', { env }, (err, stdout, stderr) => {
if (err) {
done(err)
Expand Down
90 changes: 3 additions & 87 deletions packages/server/test/integration/cypress_spec.js
View file
Open in desktop
Expand Up @@ -473,33 +473,6 @@ describe('lib/cypress', () => {
})
})

// NOTE: Removal of fixtures is not supported in new flow
it.skip('removes fixtures when they exist and fixturesFolder is false', function (done) {
ctx.actions.project.setCurrentProjectAndTestingTypeForTestSetup(this.idsPath)

ctx.lifecycleManager.getFullInitialConfig()
.then((cfg) => {
this.cfg = cfg

return fs.statAsync(this.cfg.fixturesFolder)
}).then(() => {
return settings.read(this.idsPath)
}).then((json) => {
json.fixturesFolder = false

return settings.writeForTesting(this.idsPath, json)
}).then(() => {
return cypress.start([`--run-project=${this.idsPath}`])
}).then(() => {
return fs.statAsync(this.cfg.fixturesFolder)
.then(() => {
throw new Error('fixturesFolder should not exist!')
}).catch(() => {
return done()
})
})
})

it('runs project headlessly and displays gui', function () {
return cypress.start([`--run-project=${this.todosPath}`, '--headed'])
.then(() => {
Expand Down Expand Up @@ -727,13 +700,12 @@ describe('lib/cypress', () => {
})
})

// TODO: test this
it.skip('logs error and exits when project has cypress.config.js syntax error', function () {
it('logs error and exits when project has cypress.config.js syntax error', function () {
return fs.writeFileAsync(`${this.todosPath}/cypress.config.js`, `module.exports = {`)
.then(() => {
return cypress.start([`--run-project=${this.todosPath}`])
}).then(() => {
this.expectExitWithErr('ERROR_READING_FILE', this.todosPath)
this.expectExitWithErr('CONFIG_FILE_REQUIRE_ERROR', this.todosPath)
})
})

Expand Down Expand Up @@ -1143,8 +1115,7 @@ describe('lib/cypress', () => {
})

describe('--config-file', () => {
// TODO: fix
it.skip(`with a custom config file fails when it doesn't exist`, function () {
it(`with a custom config file fails when it doesn't exist`, function () {
this.filename = 'abcdefgh.test.js'

return fs.statAsync(path.join(this.todosPath, this.filename))
Expand Down Expand Up @@ -1729,61 +1700,6 @@ describe('lib/cypress', () => {
})
})
})

// NOTE: skipped because we want to ensure this is captured in v10
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This workflow is covered by a system test system-tests/test/network_error_handling_spec.js

Copy link
Contributor

Choose a reason for hiding this comment

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

We also have it here for launchpad:

https://github.com/cypress-io/cypress/blob/develop/packages/launchpad/cypress/e2e/config-files-error-handling.cy.ts#L242-L271

it.skip('sends warning when baseUrl cannot be verified', function () {
const bus = new EE()
const event = { sender: { send: sinon.stub() } }
const warning = { message: 'Blah blah baseUrl blah blah' }

sinon.stub(ServerE2E.prototype, 'open').resolves([2121, warning])

return cypress.start(['--port=2121', '--config', 'pageLoadTimeout=1000', '--foo=bar', '--env=baz=baz'])
.then(() => {
const options = Events.start.firstCall.args[0]

Events.handleEvent(options, bus, event, 123, 'on:project:warning')

return Events.handleEvent(options, bus, event, 123, 'open:project', this.todosPath)
}).then(() => {
expect(event.sender.send.withArgs('response').firstCall.args[1].data).to.eql(warning)
})
})

describe('--config-file', () => {
beforeEach(function () {
this.filename = 'foo.bar.baz.asdf.quux.json'
this.open = sinon.stub(ServerE2E.prototype, 'open').resolves([])
})

// TODO: (tgriesser) needs a system test, the mocking here no longer is correct
it.skip('reads config from a custom config file', function () {
const bus = new EE()

return fs.writeJson(path.join(this.pristinePath, this.filename), {
env: { foo: 'bar' },
port: 2020,
}).then(() => {
return cypress.start([
`--config-file=${this.filename}`,
])
.then(() => {
const options = Events.start.firstCall.args[0]

return Events.handleEvent(options, bus, {}, 123, 'open:project', this.pristinePath)
})
.then(() => {
expect(this.open).to.be.called

const cfg = this.open.getCall(0).args[0]

expect(cfg.env.foo).to.equal('bar')

expect(cfg.port).to.equal(2020)
})
})
})
})
})

context('--cwd', () => {
Expand Down
7 changes: 0 additions & 7 deletions packages/server/test/unit/browsers/firefox_spec.ts
View file
Open in desktop
Expand Up @@ -279,13 +279,6 @@ describe('lib/browsers/firefox', () => {
})
})

// TODO: pick open port for debugger
it.skip('finds remote port for firefox debugger', function () {
return firefox.open(this.browser, 'http://', this.options, this.automation).then(() => {
// expect(firefoxUtil.findRemotePort).to.be.called
Copy link
Contributor Author

Choose a reason for hiding this comment

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

findRemotePort doesn't exist in firefoxUtil anymore

})
})

it('sets proxy-related preferences if specified', function () {
this.options.proxyServer = 'http://proxy-server:1234'

Expand Down
43 changes: 15 additions & 28 deletions packages/server/test/unit/config_spec.js
View file
Open in desktop
Expand Up @@ -11,6 +11,8 @@ const config = require(`../../lib/config`)
const errors = require(`../../lib/errors`)
const configUtil = require(`../../lib/util/config`)

const os = require('node:os')

describe('lib/config', () => {
before(function () {
this.env = process.env
Expand Down Expand Up @@ -157,18 +159,10 @@ describe('lib/config', () => {
return this.expectValidationPasses()
})

// NOTE: Validated in real use
it.skip('validates cypress.config.js', function () {
it('validates cypress.config.js', function () {
this.setup({ reporter: 5 })

return this.expectValidationFails('cypress.config.{js,ts,mjs,cjs}')
})

// NOTE: Validated in real use
it.skip('validates cypress.env.json', function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a system test for cypress.env.json validation

this.setup({}, { reporter: 5 })

return this.expectValidationFails('cypress.env.json')
return this.expectValidationFails('Expected reporter to be a string')
})

it('only validates known values', function () {
Expand Down Expand Up @@ -578,27 +572,27 @@ describe('lib/config', () => {
})

// TODO:(lachlan): after mega PR
Copy link
Contributor

Choose a reason for hiding this comment

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

The TODO has been addressed, should be safe to remove at this point.

context.skip('specPattern', () => {
context('specPattern', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to point out that fixing this test uncovered that we aren't validating the specPattern property in our latest version. This would be an issue if a user had a specPattern in their config that wasn't either a string or an array of strings. For example, if I had

specPattern: 5

in my configuration, I would see this error
Screen Shot 2022-06-17 at 12 27 02 PM

now, I would see this error

Screen Shot 2022-06-17 at 12 25 47 PM

Copy link
Contributor

Choose a reason for hiding this comment

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

Lol, as soon as we skipped a test, the functionality regressed and no-one noticed. Obvious lesson is don't skip tests!!!!! 🤦

it('passes if a string', function () {
this.setup({ e2e: { specPattern: '**/*.coffee' } })
this.setup({ e2e: { supportFile: false, specPattern: '**/*.coffee' } })

return this.expectValidationPasses()
})

it('passes if an array of strings', function () {
this.setup({ e2e: { specPattern: ['**/*.coffee'] } })
this.setup({ e2e: { supportFile: false, specPattern: ['**/*.coffee'] } })

return this.expectValidationPasses()
})

it('fails if not a string or array', function () {
this.setup({ e2e: { specPattern: 42 } })
this.setup({ e2e: { supportFile: false, specPattern: 42 } })

return this.expectValidationFails('be a string or an array of strings')
})

it('fails if not an array of strings', function () {
this.setup({ e2e: { specPattern: [5] } })
this.setup({ e2e: { supportFile: false, specPattern: [5] } })

return this.expectValidationFails('be a string or an array of strings')
.then(() => {
Expand Down Expand Up @@ -1466,8 +1460,7 @@ describe('lib/config', () => {
expect(warning).to.be.calledWith('FIREFOX_GC_INTERVAL_REMOVED')
})

// TODO:(lachlan) after mega PR
describe.skip('.resolved', () => {
describe('.resolved', () => {
it('sets reporter and port to cli', () => {
const obj = {
projectRoot: '/foo/bar',
Expand All @@ -1483,22 +1476,20 @@ describe('lib/config', () => {
.then((cfg) => {
expect(cfg.resolved).to.deep.eq({
animationDistanceThreshold: { value: 5, from: 'default' },
arch: { value: os.arch(), from: 'default' },
baseUrl: { value: null, from: 'default' },
blockHosts: { value: null, from: 'default' },
browsers: { value: [], from: 'default' },
chromeWebSecurity: { value: true, from: 'default' },
clientCertificates: { value: [], from: 'default' },
component: { from: 'default', value: {} },
defaultCommandTimeout: { value: 4000, from: 'default' },
downloadsFolder: { value: 'cypress/downloads', from: 'default' },
e2e: { from: 'default', value: {} },
env: {},
execTimeout: { value: 60000, from: 'default' },
experimentalFetchPolyfill: { value: false, from: 'default' },
experimentalInteractiveRunEvents: { value: false, from: 'default' },
experimentalSessionAndOrigin: { value: false, from: 'default' },
experimentalSourceRewriting: { value: false, from: 'default' },
experimentalStudio: { value: false, from: 'default' },
fileServerFolder: { value: '', from: 'default' },
fixturesFolder: { value: 'cypress/fixtures', from: 'default' },
hosts: { value: null, from: 'default' },
Expand All @@ -1509,6 +1500,7 @@ describe('lib/config', () => {
modifyObstructiveCode: { value: true, from: 'default' },
numTestsKeptInMemory: { value: 50, from: 'default' },
pageLoadTimeout: { value: 60000, from: 'default' },
platform: { value: os.platform(), from: 'default' },
port: { value: 1234, from: 'cli' },
projectId: { value: null, from: 'default' },
redirectionLimit: { value: 20, from: 'default' },
Expand All @@ -1525,7 +1517,6 @@ describe('lib/config', () => {
supportFile: { value: false, from: 'config' },
supportFolder: { value: false, from: 'default' },
taskTimeout: { value: 60000, from: 'default' },
specPattern: { value: '**/*.*', from: 'default' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this get removed?

trashAssetsBeforeRuns: { value: true, from: 'default' },
userAgent: { value: null, from: 'default' },
video: { value: true, from: 'default' },
Expand Down Expand Up @@ -1570,22 +1561,20 @@ describe('lib/config', () => {
return config.mergeDefaults(obj, options)
.then((cfg) => {
expect(cfg.resolved).to.deep.eq({
arch: { value: os.arch(), from: 'default' },
animationDistanceThreshold: { value: 5, from: 'default' },
baseUrl: { value: 'http://localhost:8080', from: 'config' },
blockHosts: { value: null, from: 'default' },
browsers: { value: [], from: 'default' },
chromeWebSecurity: { value: true, from: 'default' },
component: { from: 'default', value: {} },
clientCertificates: { value: [], from: 'default' },
defaultCommandTimeout: { value: 4000, from: 'default' },
downloadsFolder: { value: 'cypress/downloads', from: 'default' },
e2e: { from: 'default', value: {} },
execTimeout: { value: 60000, from: 'default' },
experimentalFetchPolyfill: { value: false, from: 'default' },
experimentalInteractiveRunEvents: { value: false, from: 'default' },
experimentalSessionAndOrigin: { value: false, from: 'default' },
experimentalSourceRewriting: { value: false, from: 'default' },
experimentalStudio: { value: false, from: 'default' },
env: {
foo: {
value: 'foo',
Expand Down Expand Up @@ -1618,6 +1607,7 @@ describe('lib/config', () => {
modifyObstructiveCode: { value: true, from: 'default' },
numTestsKeptInMemory: { value: 50, from: 'default' },
pageLoadTimeout: { value: 60000, from: 'default' },
platform: { value: os.platform(), from: 'default' },
port: { value: 2020, from: 'config' },
projectId: { value: 'projectId123', from: 'env' },
redirectionLimit: { value: 20, from: 'default' },
Expand All @@ -1634,7 +1624,6 @@ describe('lib/config', () => {
supportFile: { value: false, from: 'config' },
supportFolder: { value: false, from: 'default' },
taskTimeout: { value: 60000, from: 'default' },
specPattern: { value: '**/*.*', from: 'default' },
Copy link
Contributor

Choose a reason for hiding this comment

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

Right, this is no longer top level? Maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I believe so

trashAssetsBeforeRuns: { value: true, from: 'default' },
userAgent: { value: null, from: 'default' },
video: { value: true, from: 'default' },
Expand Down Expand Up @@ -1861,9 +1850,7 @@ describe('lib/config', () => {
})
})

// TODO: Figure out the behavior on updateWithPluginValues, should we check
// the config from cfg, or get it from the data-context?
it.skip('catches browsers=null returned from plugins', () => {
it('catches browsers=null returned from plugins', () => {
const browser = {
name: 'fake browser name',
family: 'chromium',
Expand Down