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

Version warning when local version is behind #1407

Merged
merged 2 commits into from Oct 9, 2017

Conversation

Projects
None yet
3 participants
@timdeschryver
Contributor

timdeschryver commented Jun 10, 2017

Fixes #1336

Added variable process.env.AVA_LOCAL_CLI to show the correct message.

@novemberborn

Thanks for opening the PR @tdeschryver. There's some subtle logic issues. Might be worth stubbing the update-notifier module in lib/cli.js to verify the isGlobal value.

We've been having some trouble with the CI so I don't think those failures are due to this PR.

Show outdated Hide outdated cli.js
Show outdated Hide outdated lib/cli.js
@timdeschryver

This comment has been minimized.

Show comment
Hide comment
@timdeschryver

timdeschryver Jun 10, 2017

Contributor

@novemberborn I was trying to figure out how I could test the isGlobal value and to be honest I'm kinda stuck. The only way to test it would be to call lib/cli directly and stub almost everything (update-notifier, pkg-conf, api, ...), and put a test inside the .notify() function?

Contributor

timdeschryver commented Jun 10, 2017

@novemberborn I was trying to figure out how I could test the isGlobal value and to be honest I'm kinda stuck. The only way to test it would be to call lib/cli directly and stub almost everything (update-notifier, pkg-conf, api, ...), and put a test inside the .notify() function?

@novemberborn

This comment has been minimized.

Show comment
Hide comment
@novemberborn

novemberborn Jun 11, 2017

Member

@tdeschryver in your current test, try this:

const notifySpy = sinon.spy()

proxyquire('../cli', {
  debug: debugStub,
  'resolve-cwd': resolveCwdStub,
  './lib/cli': proxyquire('../lib/cli', {
    'update-notifier' () {
      return {notify: notifySpy}
    }
  })
});

You should then be able to assert on the notifySpy arguments.

Member

novemberborn commented Jun 11, 2017

@tdeschryver in your current test, try this:

const notifySpy = sinon.spy()

proxyquire('../cli', {
  debug: debugStub,
  'resolve-cwd': resolveCwdStub,
  './lib/cli': proxyquire('../lib/cli', {
    'update-notifier' () {
      return {notify: notifySpy}
    }
  })
});

You should then be able to assert on the notifySpy arguments.

@timdeschryver

This comment has been minimized.

Show comment
Hide comment
@timdeschryver

timdeschryver Jun 11, 2017

Contributor

@novemberborn I don't think that will work (or I am missing something), the problem is we are stubbing resolveCwd with /fixture/empty, thus the lib/cli is never being called. I tried creating a new test where lib/cli is being called, the problem here is that it will test some files and to ignore this we'll have to stub a lot of things I think.

EDIT: just tried this and the notify function isn't being called as far as I can see.

Contributor

timdeschryver commented Jun 11, 2017

@novemberborn I don't think that will work (or I am missing something), the problem is we are stubbing resolveCwd with /fixture/empty, thus the lib/cli is never being called. I tried creating a new test where lib/cli is being called, the problem here is that it will test some files and to ignore this we'll have to stub a lot of things I think.

EDIT: just tried this and the notify function isn't being called as far as I can see.

@novemberborn

This comment has been minimized.

Show comment
Hide comment
@novemberborn

novemberborn Jun 11, 2017

Member

@tdeschryver could you push your attempt? I'll try on my end.

Member

novemberborn commented Jun 11, 2017

@tdeschryver could you push your attempt? I'll try on my end.

@timdeschryver

This comment has been minimized.

Show comment
Hide comment
@timdeschryver

timdeschryver Jun 11, 2017

Contributor

@novemberborn I discarded my changes, would it help you if I rewrite it?

Contributor

timdeschryver commented Jun 11, 2017

@novemberborn I discarded my changes, would it help you if I rewrite it?

@novemberborn

This comment has been minimized.

Show comment
Hide comment
@novemberborn

novemberborn Jun 17, 2017

Member

@tdeschryver yea I'll try and give this a go. #1376 landing also threw a wrench into the works.

Member

novemberborn commented Jun 17, 2017

@tdeschryver yea I'll try and give this a go. #1376 landing also threw a wrench into the works.

@novemberborn

This comment has been minimized.

Show comment
Hide comment
@novemberborn

novemberborn Jun 18, 2017

Member

@sindresorhus import-local breaks the approach taken in this PR. We need to set AVA_LOCAL_CLI before the local CLI module is required. import-local requires the module immediately. Perhaps it could export an existsSync method?

Member

novemberborn commented Jun 18, 2017

@sindresorhus import-local breaks the approach taken in this PR. We need to set AVA_LOCAL_CLI before the local CLI module is required. import-local requires the module immediately. Perhaps it could export an existsSync method?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Jun 26, 2017

Member

I think we're trying to solve this problem in the wrong place. Better to fix it in update-notifier so it's fixed for everyone. See: yeoman/update-notifier#112

Member

sindresorhus commented Jun 26, 2017

I think we're trying to solve this problem in the wrong place. Better to fix it in update-notifier so it's fixed for everyone. See: yeoman/update-notifier#112

@timdeschryver

This comment has been minimized.

Show comment
Hide comment
@timdeschryver

timdeschryver Jun 26, 2017

Contributor

@sindresorhus is it OK if I'm going to take a look at it?

Contributor

timdeschryver commented Jun 26, 2017

@sindresorhus is it OK if I'm going to take a look at it?

@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Jun 26, 2017

Member

@tdeschryver Go ahead 😃

Member

sindresorhus commented Jun 26, 2017

@tdeschryver Go ahead 😃

@timdeschryver

This comment has been minimized.

Show comment
Hide comment
@timdeschryver

timdeschryver Oct 9, 2017

Contributor

Bumped update-notifier since yeoman/update-notifier#114 is merged.

Contributor

timdeschryver commented Oct 9, 2017

Bumped update-notifier since yeoman/update-notifier#114 is merged.

@sindresorhus sindresorhus merged commit bb91862 into avajs:master Oct 9, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 42e7c74...6fc828d
Details
codecov/project 97.01% remains the same compared to 42e7c74
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sindresorhus

This comment has been minimized.

Show comment
Hide comment
@sindresorhus

sindresorhus Oct 9, 2017

Member

Thanks @tdeschryver :)

Member

sindresorhus commented Oct 9, 2017

Thanks @tdeschryver :)

@timdeschryver timdeschryver deleted the timdeschryver:localcli branch Oct 9, 2017

novemberborn added a commit that referenced this pull request Oct 10, 2017

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