ronantakizawa/codereview-bench huggingface dataset:
Repository: cypress-io/cypress
File: cli/lib/cli.js (JavaScript)

Code before:
    .option('list', text('cacheList'))
    .option('path', text('cachePath'))
    .option('clear', text('cacheClear'))
    .option('prune', text('cachePrune'))
    .option('--size', text('cacheSize'))
    .action(function (opts, args) {
      if (!args || !args.length) {
        this.outputHelp()
        util.exit(1)
      }

      const [command] = args

      if (!_.includes(['list', 'path', 'clear', 'prune'], command)) {
        unknownOption.call(this, `cache ${command}`, 'command')
      }

      if (command === 'list') {
        debug('cache command %o', {
          command,
          size: opts.size,
        })

        return cache.list(opts.size)
        .catch({ code: 'ENOENT' }, () => {
          logger.always('No binary cached versions were found.')
          process.exit(0)
        })
        .catch((e) => {
          debug('cache list command failed with "%s"', e.message)

          util.logErrorExit1(e)
        })
      }

      cache[command]()
    })

    program
    .command('info')
    .usage('[command]')
    .description('Prints Cypress and system information')
    .option('--dev', text('dev'), coerceFalse)
    .action((opts) => {
      require('./exec/info')
      .start(opts)
      .then(util.exit)
      .catch(util.logErrorExit1)
    })

    debug('cli starts with arguments %j', args)

Review comment (performance):
```suggestion
          logger.always('No cached binary versions were found.')
```

Code after:
    .option('list', text('cacheList'))
    .option('path', text('cachePath'))
    .option('clear', text('cacheClear'))
    .option('prune', text('cachePrune'))
    .option('--size', text('cacheSize'))
    .action(function (opts, args) {
      if (!args || !args.length) {
        this.outputHelp()
        util.exit(1)
      }

      const [command] = args

      if (!_.includes(['list', 'path', 'clear', 'prune'], command)) {
        unknownOption.call(this, `cache ${command}`, 'command')
      }

      if (command === 'list') {
        debug('cache command %o', {
          command,
          size: opts.size,
        })

        return cache.list(opts.size)
        .catch({ code: 'ENOENT' }, () => {
          logger.always('No cached binary versions were found.')
          process.exit(0)
        })
        .catch((e) => {
          debug('cache list command failed with "%s"', e.message)

          util.logErrorExit1(e)
        })
      }

      cache[command]()
    })

    program
    .command('info')
    .usage('[command]')
    .description('Prints Cypress and system information')
    .option('--dev', text('dev'), coerceFalse)
    .action((opts) => {
      require('./exec/info')
      .start(opts)
      .then(util.exit)
      .catch(util.logErrorExit1)
    })

    debug('cli starts with arguments %j', args)
Repository: cypress-io/cypress
File: cli/__snapshots__/cli_spec.js (JavaScript)

Code before:
    install [options]  Installs the Cypress executable matching this package's
                       version
    verify [options]   Verifies that Cypress is installed correctly and
                       executable
    cache [options]    Manages the Cypress binary cache
    info [options]     Prints Cypress and system information
  -------
  stderr:
  -------
  
  -------

`

exports['cli version and binary version with npm log silent'] = `
Cypress package version: 1.2.3
Cypress binary version: X.Y.Z
`

exports['cli version and binary version with npm log warn'] = `
Cypress package version: 1.2.3
Cypress binary version: X.Y.Z
`

exports['prints explanation when no cache'] = `
No binary cached versions were found.
`


Review comment (performance):
```suggestion
No cached binary versions were found.
```

Code after:
    install [options]  Installs the Cypress executable matching this package's
                       version
    verify [options]   Verifies that Cypress is installed correctly and
                       executable
    cache [options]    Manages the Cypress binary cache
    info [options]     Prints Cypress and system information
  -------
  stderr:
  -------
  
  -------

`

exports['cli version and binary version with npm log silent'] = `
Cypress package version: 1.2.3
Cypress binary version: X.Y.Z
`

exports['cli version and binary version with npm log warn'] = `
Cypress package version: 1.2.3
Cypress binary version: X.Y.Z
`

exports['prints explanation when no cache'] = `
No cached binary versions were found.
`
Repository: cypress-io/cypress
File: packages/server-ct/crossword-example/unit/App.spec.js (JavaScript)

Code before:
import { mount } from '@vue/test-utils'
import App from '@/App'
import { crossword } from '../cypress/fixtures/crosswords'
import { fillCrossword, getCrossword } from './jestHelpers'

// eslint-disable-next-line no-undef
global.fetch = jest.fn(() => {
  return Promise.resolve({
    json: () => Promise.resolve({}),
  })
})

fetch.mockImplementationOnce(() => {
  return Promise.resolve({
    json: () => Promise.resolve(crossword),
  })
})

beforeEach(() => {
  fetch.mockClear()
})

describe('App', () => {
  let wrapper

  beforeEach(async () => {
    fetch.mockImplementationOnce(() => {
      return Promise.resolve({
        json: () => Promise.resolve(crossword),
      })
    })

Review comment (suggestion):
This should be `/* global jest */` at the top instead of disabling `no-undef`.

Code after:
/* global jest */
import { mount } from '@vue/test-utils'
import App from '@/App'
import { crossword } from '../cypress/fixtures/crosswords'
import { fillCrossword, getCrossword } from './jestHelpers'

global.fetch = jest.fn(() => {
  return Promise.resolve({
    json: () => Promise.resolve({}),
  })
})

fetch.mockImplementationOnce(() => {
  return Promise.resolve({
    json: () => Promise.resolve(crossword),
  })
})

beforeEach(() => {
  fetch.mockClear()
})

describe('App', () => {
  let wrapper

  beforeEach(async () => {
    fetch.mockImplementationOnce(() => {
      return Promise.resolve({
        json: () => Promise.resolve(crossword),
      })
    })
Repository: cypress-io/cypress
File: cli/lib/exec/open.js (JavaScript)

Code before:

    if (options.config) {
      args.push('--config', options.config)
    }

    if (options.configFile !== undefined) {
      args.push('--config-file', options.configFile)
    }

    if (options.browser) {
      args.push('--browser', options.browser)
    }

    if (options.env) {
      args.push('--env', options.env)
    }

    if (options.port) {
      args.push('--port', options.port)
    }

    if (options.project) {
      args.push('--project', options.project)
    }

    processTestingType(options.testingType, args)

    debug('opening from options %j', options)
    debug('command line arguments %j', args)

    function open () {
      return spawn.start(args, {
        dev: options.dev,
        detached: Boolean(options.detached),
        stdio: 'inherit',
      })
    }

    if (options.dev) {
      return open()
    }

    return verify.start()
    .then(open)
  },
}


Review comment (suggestion):
Instead of mutating the args in this function, maybe we could return the new arg then push it in?

```js
const testingTypeArg = processTestingType(options.testingType, args)
args.push(testingTypeArg)
```

I find these side-effects (eg, mutating an function argument that is passed by reference) can be difficult to follow - you need to dive into the function and keep a mental model of what and where things are mutated.

Code after:

    if (options.config) {
      args.push('--config', options.config)
    }

    if (options.configFile !== undefined) {
      args.push('--config-file', options.configFile)
    }

    if (options.browser) {
      args.push('--browser', options.browser)
    }

    if (options.env) {
      args.push('--env', options.env)
    }

    if (options.port) {
      args.push('--port', options.port)
    }

    if (options.project) {
      args.push('--project', options.project)
    }

    args.push(...processTestingType(options.testingType))

    debug('opening from options %j', options)
    debug('command line arguments %j', args)

    function open () {
      return spawn.start(args, {
        dev: options.dev,
        detached: Boolean(options.detached),
        stdio: 'inherit',
      })
    }

    if (options.dev) {
      return open()
    }

    return verify.start()
    .then(open)
  },
}
Repository: cypress-io/cypress
File: packages/driver/cypress/integration/cypress/currentTest_spec.js (JavaScript)

Code before:
let global_err = null

try {
  Cypress.currentTest
} catch (e) {
  global_err = e
}

describe('currentTest', () => {
  let suite_err = null

  try {
    Cypress.currentTest
  } catch (e) {
    suite_err = e
  }

  before(() => {
    expect(Cypress.currentTest.title).eq(cy.state('runnable').ctx.currentTest.title)
  })

  beforeEach(() => {
    expect(Cypress.currentTest.title).eq(cy.state('runnable').ctx.currentTest.title)
  })

  after(() => {
    expect(Cypress.currentTest.title).eq(cy.state('runnable').ctx.currentTest.title)
  })

  afterEach(() => {
    expect(Cypress.currentTest.title).eq(cy.state('runnable').ctx.currentTest.title)
  })

  it('returns current test runnable', () => {
    expect(Cypress.currentTest.title).eq(cy.state('runnable').title)
  })

  context('errors', () => {
    it('throws if outside test', () => {
      expect(global_err).property('message').contain('outside a test')
      expect(suite_err).property('message').contain('outside a test')
    })

    it('throws if outside test with codeframe', (done) => {

Review comment (refactor):
This will pass even if both are `undefined`.

```suggestion
	expect(Cypress.currentTest.title).to.be.a('string')
    expect(Cypress.currentTest.title).eq(cy.state('runnable').ctx.currentTest.title)
```

... and move this to a function and do the same in the other hooks?

Code after:
const rootResult = Cypress.currentTest

describe('currentTest', () => {
  const suiteResult = Cypress.currentTest

  before(() => {
    expectMatchingCurrentTitleInHook()
  })

  beforeEach(() => {
    expectMatchingCurrentTitleInHook()
  })

  after(() => {
    expectMatchingCurrentTitleInHook()
  })

  afterEach(() => {
    expectMatchingCurrentTitleInHook()
  })

  it('returns current test runnable properties', () => {
    expect(Cypress.currentTest.title)
    .is.a('string')
    .eq(cy.state('runnable').title)
    .eq('returns current test runnable properties')

    expect(Cypress.currentTest.titlePath)
    .deep.eq(cy.state('runnable').titlePath())
  })

  it('returns null when outside test', () => {
    expect(rootResult).eq(null)
    expect(suiteResult).eq(null)
  })
})

const expectMatchingCurrentTitleInHook = () => {
  expect(Cypress.currentTest.title)
  .is.a('string')
  .eq(cy.state('runnable').ctx.currentTest.title)

  expect(Cypress.currentTest.titlePath)
  .deep.eq(cy.state('runnable').ctx.currentTest.titlePath())
Repository: cypress-io/cypress
File: packages/driver/src/cypress.js (JavaScript)

Code before:

  stop () {
    if (!this.runner) {
      // the tests have been reloaded
      return
    }

    this.runner.stop()
    this.cy.stop()

    return this.action('cypress:stop')
  }

  addAssertionCommand () {
    return throwPrivateCommandInterface('addAssertionCommand')
  }

  addUtilityCommand () {
    return throwPrivateCommandInterface('addUtilityCommand')
  }

  get currentTest () {
    const r = this.cy.state('runnable')

    if (!r) {
      const invocationStack = $stackUtils.getInvocationDetails(this.cy.state('specWindow'), this.config)?.stack

      throw errByPath(errs.currentTest.outside_test)
      .setUserInvocationStack(invocationStack)
    }

    // if we're in a hook, ctx.currentTest is defined
    // if we're in test body, r is the currentTest
    return r.ctx.currentTest || r
  }

  static create (config) {
    return new $Cypress(config)
  }
}

// attach to $Cypress to access
// all of the constructors
// to enable users to monkeypatch
$Cypress.prototype.$Cypress = $Cypress
$Cypress.prototype.Cy = $Cy
$Cypress.prototype.Chainer = $Chainer
$Cypress.prototype.Cookies = $Cookies
$Cypress.prototype.Command = $Command
$Cypress.prototype.Commands = $Commands
$Cypress.prototype.dom = $dom

Review comment (suggestion):
Might be more straightforward to set `this.specWindow` in `onSpecWindow` instead of using `cy.state`.

Code after:

        return resolve(reply.response)
      }

      return this.emit('automation:request', eventName, ...args, fn)
    })
  }

  stop () {
    if (!this.runner) {
      // the tests have been reloaded
      return
    }

    this.runner.stop()
    this.cy.stop()

    return this.action('cypress:stop')
  }

  addAssertionCommand () {
    return throwPrivateCommandInterface('addAssertionCommand')
  }

  addUtilityCommand () {
    return throwPrivateCommandInterface('addUtilityCommand')
  }

  get currentTest () {
    const r = this.cy.state('runnable')

    if (!r) {
      return null
    }

    // if we're in a hook, ctx.currentTest is defined
    // if we're in test body, r is the currentTest
    /**
     * @type {Mocha.Test}
     */
    const currentTestRunnable = r.ctx.currentTest || r

    return {
      title: currentTestRunnable.title,
      titlePath: currentTestRunnable.titlePath(),
    }
  }

  static create (config) {
    return new $Cypress(config)
  }
Repository: cypress-io/cypress
File: packages/driver/cypress/integration/cypress/currentTest_spec.js (JavaScript)

Code before:
const rootResult = Cypress.currentTest

describe('currentTest', () => {
  const suiteResult = Cypress.currentTest

  before(() => {
    expectMatchingCurrentTitleInHook()
  })

  beforeEach(() => {
    expectMatchingCurrentTitleInHook()
  })

  after(() => {
    expectMatchingCurrentTitleInHook()
  })

  afterEach(() => {
    expectMatchingCurrentTitleInHook()
  })

  it('returns current test runnable properties', () => {
    expect(Cypress.currentTest.title)
    .is.a('string')
    .eq(cy.state('runnable').title)

    expect(Cypress.currentTest.titlePath)
    .deep.eq(cy.state('runnable').titlePath())
  })

  it('returns null when outside test', () => {
    expect(rootResult).eq(null)
    expect(suiteResult).eq(null)
  })
})

const expectMatchingCurrentTitleInHook = () => {
  expect(Cypress.currentTest.title)
  .is.a('string')
  .eq(cy.state('runnable').ctx.currentTest.title)

  expect(Cypress.currentTest.titlePath)
  .deep.eq(cy.state('runnable').ctx.currentTest.titlePath())
}


Review comment (bug):
`title` could be the same wrong string for both, so this test could use an assertion against a string literal to be more regression-proof

```suggestion
    .eq(cy.state('runnable').title)
    .eq('returns current test runnable properties')

```

Code after:
const rootResult = Cypress.currentTest

describe('currentTest', () => {
  const suiteResult = Cypress.currentTest

  before(() => {
    expectMatchingCurrentTitleInHook()
  })

  beforeEach(() => {
    expectMatchingCurrentTitleInHook()
  })

  after(() => {
    expectMatchingCurrentTitleInHook()
  })

  afterEach(() => {
    expectMatchingCurrentTitleInHook()
  })

  it('returns current test runnable properties', () => {
    expect(Cypress.currentTest.title)
    .is.a('string')
    .eq(cy.state('runnable').title)
    .eq('returns current test runnable properties')

    expect(Cypress.currentTest.titlePath)
    .deep.eq(cy.state('runnable').titlePath())
  })

  it('returns null when outside test', () => {
    expect(rootResult).eq(null)
    expect(suiteResult).eq(null)
  })
})

const expectMatchingCurrentTitleInHook = () => {
  expect(Cypress.currentTest.title)
  .is.a('string')
  .eq(cy.state('runnable').ctx.currentTest.title)

  expect(Cypress.currentTest.titlePath)
  .deep.eq(cy.state('runnable').ctx.currentTest.titlePath())
}
Repository: cypress-io/cypress
File: packages/runner/src/app/app.jsx (JavaScript)

Code before:
      })
    }

    $(win).on('resize', this._onWindowResize).trigger('resize')
  }

  _onSpecListResizeStart = () => {
    this.isSpecListResizing = true
  }

  _onReporterResizeStart = () => {
    this.isReporterResizing = true
  }

  _onSpecListResize = (specListWidth) => {
    this.props.state.specListWidth = specListWidth
    this.props.state.absoluteSpecListWidth = specListWidth

    const $header = $(findDOMNode(this.refs.header))

    this.props.state.updateWindowDimensions({
      headerHeight: $header.outerHeight(),
    })
  }

  _onReporterResize = (reporterWidth) => {
    this.props.state.reporterWidth = reporterWidth - this.props.state.specListWidth
    this.props.state.absoluteReporterWidth = reporterWidth - this.props.state.specListWidth

    const $header = $(findDOMNode(this.refs.header))

    this.props.state.updateWindowDimensions({
      headerHeight: $header.outerHeight(),
    })
  }

  _onReporterResizeEnd = () => {
    this.isReporterResizing = false
    this.props.eventManager.saveState({
      reporterWidth: this.props.state.reporterWidth,
    })
  }

  _onSpecListResizeEnd = () => {
    this.isSpecListResizing = false
    this.props.eventManager.saveState({
      specListWidth: this.props.state.specListWidth,
    })
  }

  _handleScreenshots () {

Review comment (suggestion):
The trick is that if the feature flag is set to false, `state.specListWidth` is 0 and it's business as usual 😎

Code after:

    $(win).on('resize', this._onWindowResize).trigger('resize')
  }

  _onSpecListResizeStart = () => {
    this.isSpecListResizing = true
  }

  _onReporterResizeStart = () => {
    this.isReporterResizing = true
  }

  _onSpecListResize = (specListWidth) => {
    this.props.state.specListWidth = specListWidth
    this.props.state.absoluteSpecListWidth = specListWidth

    const $header = $(findDOMNode(this.refs.header))

    this.props.state.updateWindowDimensions({
      headerHeight: $header.outerHeight(),
    })
  }

  _onReporterResize = (reporterWidth) => {
    this.props.state.reporterWidth = reporterWidth - this.props.state.specListWidth
    this.props.state.absoluteReporterWidth = reporterWidth - this.props.state.specListWidth

    const $header = $(findDOMNode(this.refs.header))

    this.props.state.updateWindowDimensions({
      headerHeight: $header.outerHeight(),
    })
  }

  _onReporterResizeEnd = () => {
    this.isReporterResizing = false
    this.props.eventManager.saveState({
      reporterWidth: this.props.state.reporterWidth,
    })
  }

  _onSpecListResizeEnd = () => {
    this.isSpecListResizing = false
    this.props.eventManager.saveState({
      specListWidth: this.props.state.specListWidth,
    })
  }

  _handleScreenshots () {
    const containerNode = findDOMNode(this.refs.container)
    const reporterNode = this.refs.reporterWrap
Repository: cypress-io/cypress
File: packages/runner/src/main.jsx (JavaScript)

Code before:
      const config = JSON.parse(driverUtils.decodeBase64Unicode(base64Config))

      const NO_COMMAND_LOG = config.env && config.env.NO_COMMAND_LOG

      const state = new State({
        reporterWidth: NO_COMMAND_LOG ? 0 : (config.state || {}).reporterWidth,
        specs: config.specs,
        specListWidth: NO_COMMAND_LOG ? 0 : SPEC_LIST_WIDTH,
      })

      Runner.state = state
      Runner.configureMobx = configure

      const setSpecByUrlHash = () => {
        const specPath = util.integrationSpecPath()

        if (specPath) {
          state.updateSpecByUrl(specPath)
        }
      }

      setSpecByUrlHash()

      autorun(() => {
        const { spec } = state

        if (spec) {
          util.updateIntegrationSpecPath(spec.name)
        }
      })

      window.addEventListener('hashchange', setSpecByUrlHash)

      state.updateDimensions(config.viewportWidth, config.viewportHeight)

      const container = (
        <Container
          config={config}
          runner='e2e'
          state={state}
          App={App}
          NoSpec={NoSpec}
          hasSpecFile={util.hasSpecFile}
          eventManager={eventManager}
        />
      )

      render(container, el)
    })()
  },
}

Review comment (suggestion):
This is how we change the hash and run the test you selected in the spec list. Stolen from https://github.com/cypress-io/cypress/blob/develop/packages/runner-ct/src/main.jsx#L42-L61

Code after:
      const config = JSON.parse(driverUtils.decodeBase64Unicode(base64Config))

      const NO_COMMAND_LOG = config.env && config.env.NO_COMMAND_LOG
      const useInlineSpecList = (config.env || {}).CypressInternal_UseInlineSpecList

      const state = new State({
        reporterWidth: NO_COMMAND_LOG ? 0 : (config.state || {}).reporterWidth,
        specs: config.specs,
        useInlineSpecList,
        specListWidth: NO_COMMAND_LOG ? 0 : useInlineSpecList ? SPEC_LIST_WIDTH : 0,
      })

      Runner.state = state
      Runner.configureMobx = configure

      const setSpecByUrlHash = () => {
        const specPath = util.integrationSpecPath()

        if (specPath) {
          state.updateSpecByUrl(specPath)
        }
      }

      setSpecByUrlHash()

      autorun(() => {
        const { spec } = state

        if (spec) {
          util.updateIntegrationSpecPath(spec.name)
        }
      })

      window.addEventListener('hashchange', setSpecByUrlHash)

      state.updateDimensions(config.viewportWidth, config.viewportHeight)

      const container = (
        <Container
          config={config}
          runner='e2e'
          state={state}
          App={App}
          NoSpec={NoSpec}
          hasSpecFile={util.hasSpecFile}
          eventManager={eventManager}
        />
      )

      render(container, el)
    })()
Repository: cypress-io/cypress
File: packages/server/lib/gui/files.ts (TypeScript)

Code before:
import openProject from '../open_project'
import { createFile } from '../util/spec_writer'
import { showSaveDialog } from './dialog'

export const showDialogAndCreateSpec = async () => {
  const cfg = openProject.getConfig()

  const path = await showSaveDialog(cfg.integrationFolder)

  if (!path) {
    return {
      specs: null,
      path,
    }
  }

  // only create file if they selected a file
  if (path) {
    createFile(path)
  }

  // reload specs now that we've added a new file
  // we reload here so we can update ui immediately instead of
  // waiting for file watching to send updated spec list
  const specs = await openProject.getSpecs(cfg)

  return {
    specs,
    path,
  }

Review comment (suggestion):
`getConfig` no longer returns a promise..., so no more `then`, and no more `tap`, so I just made it into a regular old function, replacing `then` with `await`. 

Code after:
import openProject from '../open_project'
import { createFile } from '../util/spec_writer'
import { showSaveDialog } from './dialog'

export const showDialogAndCreateSpec = async () => {
  const cfg = openProject.getConfig()

  const path = await showSaveDialog(cfg.integrationFolder)

  if (!path) {
    return {
      specs: null,
      path,
    }
  }

  // only create file if they selected a file
  if (path) {
    await createFile(path)
  }

  // reload specs now that we've added a new file
  // we reload here so we can update ui immediately instead of
  // waiting for file watching to send updated spec list
  const specs = await openProject.getSpecs(cfg)

  return {
    specs,
    path,
  }