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

Add to deprecations spec #11117

Merged
merged 4 commits into from Nov 20, 2017

Conversation

Projects
None yet
2 participants
@codebytere
Member

codebytere commented Nov 14, 2017

Improve coverage in common/deprecate.js
This module also appears to be copied over from another project, so there are several implemented methods which we do not use or reference anywhere. I've commented them out.

To Do:

  • deprecate.rename
  • deprecate.warn

codebytere added some commits Nov 14, 2017

@codebytere codebytere requested a review from electron/reviewers as a code owner Nov 14, 2017

Show outdated Hide outdated lib/common/api/deprecate.js
deprecations.setHandler((message) => {
messages.push(message)
})

This comment has been minimized.

@ckerr

ckerr Nov 15, 2017

Member

messages is unused here and the deprecate.log() call is unrelated to the test. Looks like these are a copy-paste leftover; all that's really needed for this test is something like
deprecations.setHandler(message => {})

@ckerr

ckerr Nov 15, 2017

Member

messages is unused here and the deprecate.log() call is unrelated to the test. Looks like these are a copy-paste leftover; all that's really needed for this test is something like
deprecations.setHandler(message => {})

})
deprecate.log('this is deprecated')
assert(typeof deprecations.getHandler() === 'function')

This comment has been minimized.

@ckerr

ckerr Nov 15, 2017

Member

assert.deepEqual() here

@ckerr

ckerr Nov 15, 2017

Member

assert.deepEqual() here

assert(typeof deprecations.getHandler() === 'function')
})
it('returns a deprecation warning', () => {

This comment has been minimized.

@ckerr

ckerr Nov 15, 2017

Member

it('invokes the handler when set')

@ckerr

ckerr Nov 15, 2017

Member

it('invokes the handler when set')

deprecate.alias(nativeImage, 'createFromDataUrl', 'createFromDataURL')
assert.equal(typeof nativeImage.createFromDataUrl, 'function')
})

This comment has been minimized.

@ckerr

ckerr Nov 15, 2017

Member

This is a good start but, thinking it over a little, I think this is not a good test of the alias feature. We'd be better off confirming that warnings happen and that deprecatedMethod is routed to existingMethod:

  1. Create a local warned bool set to false
  2. Create a local `called' bool set to false
  3. Call deprecations.setHandler(message => warned = true)
  4. Create a toy object with a method that sets called to true
  5. Call deprecate.alias(toy ... to create an alias for the method
  6. Call the aliased method on the object
  7. assert that called is true
  8. assert that warned is true
@ckerr

ckerr Nov 15, 2017

Member

This is a good start but, thinking it over a little, I think this is not a good test of the alias feature. We'd be better off confirming that warnings happen and that deprecatedMethod is routed to existingMethod:

  1. Create a local warned bool set to false
  2. Create a local `called' bool set to false
  3. Call deprecations.setHandler(message => warned = true)
  4. Create a toy object with a method that sets called to true
  5. Call deprecate.alias(toy ... to create an alias for the method
  6. Call the aliased method on the object
  7. assert that called is true
  8. assert that warned is true
})
deprecate.warn('old', 'new')
assert.deepEqual(messages, [`'old' is deprecated. Use 'new' instead.`])

This comment has been minimized.

@ckerr

ckerr Nov 15, 2017

Member

This is OK but a little brittle, it'll fail if we ever change the warning format. What would you think about something like

let warnings = 0
deprecations.setHandler(message => ++warnings)
deprecate.warn('old', 'new')
assert.equal(warnings, 1)
@ckerr

ckerr Nov 15, 2017

Member

This is OK but a little brittle, it'll fail if we ever change the warning format. What would you think about something like

let warnings = 0
deprecations.setHandler(message => ++warnings)
deprecate.warn('old', 'new')
assert.equal(warnings, 1)

@codebytere codebytere changed the title from [WIP] add to deprecations spec to Add to deprecations spec Nov 20, 2017

@ckerr

ckerr approved these changes Nov 20, 2017

LGTM

@ckerr ckerr merged commit 4c04f1c into master Nov 20, 2017

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details

@codebytere codebytere deleted the add_deprecations_spec branch Dec 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment