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: [Multi-domain]: Yield subject from switchToDomain #19936

Merged
merged 25 commits into from
Feb 3, 2022
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6e68473
first pass at yielding a subject
mjhenkes Jan 27, 2022
0c3ddcb
Merge branch 'feature-multidomain' into yield-subject
mjhenkes Jan 27, 2022
4ce0ccc
Remove logs and add comments
mjhenkes Jan 27, 2022
a091a21
remove duplicate test
mjhenkes Jan 27, 2022
8e14be9
Add debug logging for aborted
mjhenkes Jan 27, 2022
34222f5
Merge branch 'feature-multidomain' into yield-subject
mjhenkes Jan 27, 2022
c5ca5c2
Apply suggestions from code review
mjhenkes Jan 28, 2022
40f2ad6
New strategy for serializing subjects
mjhenkes Jan 31, 2022
b2981e5
handle 'ran:domain:fn' event
mjhenkes Jan 31, 2022
bf6522c
Fix lint
mjhenkes Jan 31, 2022
24bbf59
Now with proxy error handling!
mjhenkes Feb 2, 2022
ffe38e7
Break yields tests out into their own file.
mjhenkes Feb 2, 2022
6a42881
updated test
mjhenkes Feb 2, 2022
19fb21c
Update packages/driver/src/cy/multi-domain/failedSerializeSubjectProx…
mjhenkes Feb 2, 2022
66f1de4
add a param for the malformed should
mjhenkes Feb 2, 2022
905eb5b
Apply suggestions from code review
mjhenkes Feb 2, 2022
2fe2f7f
update test to work cross browsers
mjhenkes Feb 2, 2022
9ff8fde
fix test strings
mjhenkes Feb 2, 2022
c834c9b
Update packages/driver/src/util/queue.ts
mjhenkes Feb 2, 2022
ee14266
optional chaining !
mjhenkes Feb 2, 2022
944f08d
Apply suggestions from code review
mjhenkes Feb 2, 2022
8ebc545
code review changes
mjhenkes Feb 2, 2022
0a041f6
Whoops
mjhenkes Feb 2, 2022
78fcb01
whoops, renamed the wrong test file
mjhenkes Feb 3, 2022
9f17a9a
Update packages/driver/src/cy/multi-domain/failedSerializeSubjectProx…
mjhenkes Feb 3, 2022
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
Original file line number Diff line number Diff line change
@@ -1,8 +1,17 @@
import _ from 'lodash'
const { assertLogLength } = require('../../../support/utils')

// @ts-ignore / session support is needed for visiting about:blank between tests
describe('multi-domain', { experimentalSessionSupport: true, experimentalMultiDomain: true }, () => {
let logs: any = []

beforeEach(() => {
logs = []

cy.on('log:added', (attrs, log) => {
logs.push(log)
})

cy.visit('/fixtures/multi-domain.html')
cy.get('a[data-cy="multi-domain-secondary-link"]').click()
})
Expand Down Expand Up @@ -278,4 +287,223 @@ describe('multi-domain', { experimentalSessionSupport: true, experimentalMultiDo

it('short circuits the secondary domain command queue when "done()" is called early')
})

describe('yields', () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Should I break these out to their own file?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

it('yields a value', () => {
cy.switchToDomain('foobar.com', () => {
cy
.get('[data-cy="dom-check"]')
.invoke('text')
}).should('equal', 'From a secondary domain')
})

it('yields the cy value even if a return is present', () => {
cy.switchToDomain('foobar.com', async () => {
cy
.get('[data-cy="dom-check"]')
.invoke('text')

const p = new Promise((resolve, reject) => {
setTimeout(() => {
resolve('text')
}, 1000)
})

return p
}).should('equal', 'From a secondary domain')
})

it('errors if a cy command is present and it returns a sync value', (done) => {
cy.on('fail', (err) => {
assertLogLength(logs, 6)
expect(logs[5].get('error')).to.eq(err)
expect(err.message).to.include('`cy.switchToDomain()` failed because you are mixing up async and sync code.')

done()
})

cy.switchToDomain('foobar.com', () => {
cy
.get('[data-cy="dom-check"]')
.invoke('text')

return 'text'
})
})

it('yields synchronously', () => {
cy.switchToDomain('foobar.com', () => {
return 'From a secondary domain'
}).should('equal', 'From a secondary domain')
})

AtofStryker marked this conversation as resolved.
Show resolved Hide resolved
it('yields asynchronously', () => {
cy.switchToDomain('foobar.com', async () => {
return new Promise((resolve: (val: string) => any, reject) => {
setTimeout(() => {
resolve('From a secondary domain')
}, 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be shortened to something like 50-100ms?

Copy link
Member Author

Choose a reason for hiding this comment

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

it sure can!

})
}).should('equal', 'From a secondary domain')
})

it('succeeds if subject cannot be serialized and is not accessed synchronously', () => {
cy.switchToDomain('foobar.com', () => {
return {
symbol: Symbol(''),
}
}).then((obj) => {
return 'object not accessed'
}).should('equal', 'object not accessed')
})

it('throws if subject cannot be serialized and is accessed synchronously', (done) => {
cy.on('fail', (err) => {
assertLogLength(logs, 6)
expect(logs[5].get('error')).to.eq(err)
expect(err.message).to.include('`cy.switchToDomain()` could not serialize the subject due to one of it\'s properties not being supported by the structured clone algorithm.')

done()
})

cy.switchToDomain('foobar.com', () => {
return {
symbol: Symbol(''),
}
}).should('equal', '')
Copy link
Contributor

@mschile mschile Feb 2, 2022

Choose a reason for hiding this comment

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

I assume the should is what's failing here but I think it might be more readable if it's more explicit in a .then

Suggested change
}).should('equal', '')
}).then((obj) => {
// This will fail accessing the symbol
return obj.symbol
}

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

})

it('succeeds if subject cannot be serialized and is not accessed', () => {
cy.switchToDomain('foobar.com', () => {
cy
.get('[data-cy="dom-check"]')
}).then((obj) => {
return 'object not accessed'
}).should('equal', 'object not accessed')
})

it('throws if subject cannot be serialized and is accessed', (done) => {
cy.on('fail', (err) => {
assertLogLength(logs, 7)
expect(logs[6].get('error')).to.eq(err)
expect(err.message).to.include('`cy.switchToDomain()` could not serialize the subject due to one of it\'s properties not being supported by the structured clone algorithm.')

done()
})

cy.switchToDomain('foobar.com', () => {
cy
.get('[data-cy="dom-check"]')
}).should('equal')
})

it('throws if an object contains a function', (done) => {
cy.on('fail', (err) => {
assertLogLength(logs, 7)
expect(logs[6].get('error')).to.eq(err)
expect(err.message).to.include('`cy.switchToDomain()` could not serialize the subject due to one of it\'s properties not being supported by the structured clone algorithm.')

done()
})

cy.switchToDomain('foobar.com', () => {
cy.wrap({
key: () => {
return 'whoops'
},
})
}).invoke('key').should('equal', 'whoops')
})

it('throws if an object contains a symbol', (done) => {
cy.on('fail', (err) => {
assertLogLength(logs, 7)
expect(logs[6].get('error')).to.eq(err)
expect(err.message).to.include('`cy.switchToDomain()` could not serialize the subject due to one of it\'s properties not being supported by the structured clone algorithm.')

done()
})

cy.switchToDomain('foobar.com', () => {
cy.wrap({
key: Symbol('whoops'),
})
}).should('equal', undefined)
})

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we want to do something similar with Errors here since they aren't serializable through the structured clone algorithm in Firefox. Since we already attempt to serialize errors to objects and back to errors in the primary, I wonder if we want to leverage the util function here in the serializer.

Or we can also just return undefined if an error is yielded to be consistent with the rest of the unserializable attributes. I'm also not sure why someone would want to yield an error 🤷‍♂️ .

it('yields undefined if an object contains an error', () => {
  cy.switchToDomain('foobar.com', () => {
    cy.wrap({
      key: new Error('Boom goes the dynamite'),
    })
  }).should('equal', undefined)
})

Copy link
Contributor

Choose a reason for hiding this comment

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

I would expect an unserializable values to be undefined, not the whole object itself. So I would think the 3 above tests would yield an empty object ({}) instead of undefined. What happens if there are other serializable values in the object like { key: undefined, otherKey: 'defined' }?

it('throws if an object is a function', (done) => {
cy.on('fail', (err) => {
assertLogLength(logs, 7)
expect(logs[6].get('error')).to.eq(err)
expect(err.message).to.include('`cy.switchToDomain()` could not serialize the subject due functions not being supported by the structured clone algorithm.')

done()
})

cy.switchToDomain('foobar.com', () => {
cy.wrap(() => {
return 'text'
})
}).then((obj) => {
// @ts-ignore
obj()
})
})

it('throws if an object is a symbol', (done) => {
cy.on('fail', (err) => {
assertLogLength(logs, 7)
expect(logs[6].get('error')).to.eq(err)
expect(err.message).to.include('`cy.switchToDomain()` could not serialize the subject due symbols not being supported by the structured clone algorithm.')

done()
})

cy.switchToDomain('foobar.com', () => {
cy.wrap(Symbol('symbol'))
}).should('equal', 'symbol')
})

// NOTE: This test will only work on chrome.
it.skip('yields an error if an object contains an error', () => {
cy.switchToDomain('foobar.com', () => {
cy.wrap({
key: new Error('Boom goes the dynamite'),
})
}).its('key.message')
.should('equal', 'Boom goes the dynamite')
})

it('yields an object containing valid types', () => {
cy.switchToDomain('foobar.com', () => {
cy.wrap({
array: [
1,
2,
],
undefined,
bool: true,
null: null,
number: 12,
object: {
key: 'key',
},
string: 'string',
})
}).should('deep.equal', {
array: [
1,
2,
],
undefined,
bool: true,
null: null,
number: 12,
object: {
key: 'key',
},
string: 'string',
})
})
})
})
12 changes: 12 additions & 0 deletions packages/driver/cypress/integration/util/queue_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,16 @@ describe('src/util/queue', () => {
expect(queue.stopped).to.false
})
})

context('.last', () => {
it('returns the last item', () => {
expect(queue.last()).to.deep.equal({ id: '3' })

queue.add({ id: '4' })
expect(queue.last()).to.deep.equal({ id: '4' })

queue.clear()
expect(queue.last()).to.equal(undefined)
})
})
})
11 changes: 9 additions & 2 deletions packages/driver/src/cy/multi-domain/commands_manager.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { PrimaryDomainCommunicator } from '../../multi-domain/communicator'
import { createDeferred, Deferred } from '../../util/deferred'
import { correctStackForCrossDomainError } from './util'
import { failedToSerializeSubject } from './failedSerializeSubjectProxy'

export class CommandsManager {
// these are proxy commands that represent real commands in a
Expand Down Expand Up @@ -57,15 +58,21 @@ export class CommandsManager {
Cypress.action('cy:enqueue:command', attrs)
}

endCommand = ({ id, name, err, logId }) => {
endCommand = ({ id, subject, failedToSerializeSubjectOfType, name, err, logId }) => {
const command = this.commands[id]

if (!command) return

delete this.commands[id]

if (!err) {
return command.deferred.resolve()
if (failedToSerializeSubjectOfType) {
return command.deferred.resolve(
failedToSerializeSubject(failedToSerializeSubjectOfType),
)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

this logic, as minimal as it is, is duplicated in index.ts (see below). Is it worth trying to do some special handling of the end:command and ran:domain:fn events in the communicator to swap in the proxy for the subject before these commands fire? (I think that could work?)


return command.deferred.resolve(subject)
}

// If the command has failed, cast the error back to a proper Error object
Expand Down
64 changes: 64 additions & 0 deletions packages/driver/src/cy/multi-domain/failedSerializeSubjectProxy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
import $errUtils from '../../cypress/error_utils'

/**
* Create a proxy object to fail when accessed or called.
* @param type The type of operand that failed to serialize
* @returns A proxy object that will fail when accessed.
*/
const failedToSerializeSubject = (type: string) => {
let target = {}

// If the failed subject is a function, use a function as the target.
if (type === 'function') {
target = () => {}
}

// Symbol note: I don't think the target can be a symbol, but we can just use an object until the symbols is accessed, then provide a different error.
mjhenkes marked this conversation as resolved.
Show resolved Hide resolved

return new Proxy(target, {
/**
* Throw an error if the proxy is called like a function.
* @param target the proxy target
* @param thisArg this
* @param argumentsList args passed.
*/
apply (target, thisArg, argumentsList) {
$errUtils.throwErrByPath('switchToDomain.failed_to_serialize_function')
},

/**
* Throw an error if any properties besides the listed ones are accessed.
* @param target The proxy target
* @param prop The property being accessed
* @param receiver Either the proxy or an object that inherits from the proxy.
* @returns either an error or the result of the allowed get on the target.
*/
get (target, prop, receiver) {
// These properties are required to avoid failing prior to attempting to use the subject.
if (
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 refactor this to a list on constants like const unserializableSubjects = ['then', 'jquery', ...] and do something like unserializableSubjects.includes(prop) since we are working with strings? It might make this a bit easier to read and maintain in the future.

prop === 'then'
|| prop === Symbol.isConcatSpreadable
|| prop === 'jquery'
|| prop === 'nodeType'
// || prop === Symbol.toStringTag // If this is passed through to the target we will not property fail the 'cy.invoke' command.
mjhenkes marked this conversation as resolved.
Show resolved Hide resolved
|| prop === 'window'
|| prop === 'document'
|| prop === 'inspect'
|| prop === 'isSinonProxy'
|| prop === '_spreadArray'
|| prop === 'selector'
Copy link
Member Author

Choose a reason for hiding this comment

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

These special props were found by trial and error. it is possible that more will have to be added as commands evolve.

) {
return target[prop]
}

// Provide a slightly different message if the object was meant to be a symbol.
if (type === 'symbol') {
$errUtils.throwErrByPath('switchToDomain.failed_to_serialize_symbol')
} else {
$errUtils.throwErrByPath('switchToDomain.failed_to_serialize_object')
}
},
})
}

export { failedToSerializeSubject }
15 changes: 12 additions & 3 deletions packages/driver/src/cy/multi-domain/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import $errUtils from '../../cypress/error_utils'
import { CommandsManager } from './commands_manager'
import { LogsManager } from './logs_manager'
import { Validator } from './validator'
import { failedToSerializeSubject } from './failedSerializeSubjectProxy'

export function addCommands (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy, state: Cypress.State, config: Cypress.InternalConfig) {
let timeoutId
Expand Down Expand Up @@ -118,7 +119,7 @@ export function addCommands (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy,
logsManager.listen()

return new Bluebird((resolve, reject) => {
communicator.once('ran:domain:fn', (err) => {
communicator.once('ran:domain:fn', ({ subject, failedToSerializeSubjectOfType, err }) => {
sendReadyForDomain()
if (err) {
if (done) {
Expand All @@ -141,9 +142,17 @@ export function addCommands (Commands, Cypress: Cypress.Cypress, cy: Cypress.cy,
// is common if there are only assertions enqueued in the SD.
if (!commandsManager.hasCommands && !done) {
cleanup()
}
// This handles when a subject is returned synchronously
if (failedToSerializeSubjectOfType) {
return resolve(
failedToSerializeSubject(failedToSerializeSubjectOfType),
)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Total nit/preference here. I'm going to apologize for the formatting in this suggestion in advance 😅

Suggested change
// This handles when a subject is returned synchronously
if (failedToSerializeSubjectOfType) {
return resolve(
failedToSerializeSubject(failedToSerializeSubjectOfType),
)
}
// This handles when a subject is returned synchronously
const serializedSubject = failedToSerializeSubjectOfType ?
failedToSerializeSubject(failedToSerializeSubjectOfType) :
subject
resolve(serializedSubject)

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated with a single line variant fo this suggestion


resolve()
resolve(subject)
} else {
resolve()
}
})

// If done is NOT passed into switchToDomain, wait for the command queue
Expand Down
Loading