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

Internal improvements, add more tests #689

Merged
merged 66 commits into from
Sep 2, 2023

Conversation

tommy-mitchell
Copy link
Collaborator

@tommy-mitchell tommy-mitchell commented Apr 8, 2023

Fixes #29
Fixes #383

Sorry for the big diff. Besides adding lots of tests, a lot of changes are pretty connected.

Summary:

  • Adds tests for almost every file
    • Utilities: npm/*, git-util.js, util.js
    • Mocks inquirer to test prompts in ui.js
    • Basic tests for cli.js and index.js
    • Uses esmock extensively to make tests concurrent
      • e.g. for stubbing execa with command outputs, or in combination with tempy for integration tests
  • Reworks the API of the Version class to be more minimal
    • Incorporates pretty-version-diff.js into Version's formatting
    • Updates source files with the new usage
  • Updates many dependencies
  • Misc. bugs/style fixes discovered while adding tests

More tests with the CLI are sorely needed.


New Version API

new Version('0.0.0').toString()
//=> '0.0.0'

new Version('0.0.0', 'major')
//=> {toString: '1.0.0', #diff: 'major'}

new Version('0.0.0').setFrom('minor').isPrerelease()
//=> false

new Version('2.0.0').satisfies('>=1.0.0')
//=> true

new Version('1.2.3', 'patch').format()
//=> '{dim 1.2.{cyan 4}}'

new Version('1.2.3-1').format({previousVersion: '1.2.3-0'})
//=> '{dim 1.2.3-{cyan 1}}'

new Version(123)
//=> throws 'Version `123` should be a valid `SemVer` version.'

new Version('1.2.3', '1.2.4')
//=> throws 'Increment `1.2.4` should be one of `major`, `minor`, `patch`, `premajor`, `preminor`, `prepatch`, `prerelease`.'

new Version('1.2.3').setFrom(124)
//=> throws 'New version `124` should either be one of `major`, `minor`, `patch`, `premajor`, `preminor`, `prepatch`, `prerelease`, or a valid `SemVer` version.'

new Version('1.2.3').setFrom('1.2.2')
//=> throws 'New version `1.2.2` should be higher than current version `1.2.3`.'

IssueHunt Summary

Referenced issues

This pull request has been submitted to:


test/npm/util/stub.js Outdated Show resolved Hide resolved
test/git/stub.js Outdated Show resolved Hide resolved
@tommy-mitchell
Copy link
Collaborator Author

#684 can be done now as well, but I think we should wait until the tests are more robust first.

@tommy-mitchell
Copy link
Collaborator Author

I think that should be everything covered in util.js as well. I also found a bug in readPkg where packagePath wasn't being properly passed to pkg-dir, which probably happened in the move to ESM with the dependency update.

@tommy-mitchell tommy-mitchell changed the title Improve/add tests, fix some style issues Internal improvements, add more tests Apr 9, 2023
@tommy-mitchell tommy-mitchell marked this pull request as ready for review August 31, 2023 16:24
@tommy-mitchell
Copy link
Collaborator Author

@sindresorhus do you want to try a test publish again? Think I've fixed the issues

@tommy-mitchell
Copy link
Collaborator Author

@sindresorhus
Copy link
Owner

~/dev/oss/sindre-playground main ⇡
❯ np

Publish a new version of sindre-playground (current: 1.7.4)

? No commits found since previous release, continue? Yes
? Select SemVer increment or specify new version major 	2.0.0

  ❯ Prerequisite check
    ✔ Ping npm registry
    ✔ Check npm version
    ✔ Check yarn version
    ✔ Verify user is authenticated
    ✔ Check git version
    ✔ Check git remote
    ✖ Validate version
      → New version `2.0.0` should either be one of `major`, `minor`, `patch`, `premajor`, `preminor`, `prepatch`, `prerelease`, or a valid `SemVer` version.
      Check for pre-release version
      Check git tag existence
    Git
    Installing dependencies using Yarn
    Running tests using Yarn
    Bumping version using Yarn
    Publishing package using Yarn
    Pushing tags
    Creating release draft on GitHub

✖ Error: New version `2.0.0` should either be one of `major`, `minor`, `patch`, `premajor`, `preminor`, `prepatch`, `prerelease`, or a valid `SemVer` version.
    at Version.setFrom (file:///Users/sindresorhus/dev/oss/np/source/version.js:93:11)
    at Task.task (file:///Users/sindresorhus/dev/oss/np/source/prerequisite-tasks.js:62:43)
    at /Users/sindresorhus/dev/oss/np/node_modules/listr/lib/task.js:167:30
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Aborted!

@tommy-mitchell
Copy link
Collaborator Author

Fixed 🙃 some e2e tests would be nice.

@sindresorhus
Copy link
Owner

A couple of minor things I noticed:

  • If I press Control+C (to exit) while it's running (for example, "Installing dependencies"), the text cursor is not restored.
  • It now presents in this order: major, minor, patch, while previously (and correctly), it presented: patch, minor, major.

source/ui.js Outdated Show resolved Hide resolved
@tommy-mitchell
Copy link
Collaborator Author

tommy-mitchell commented Sep 1, 2023

It now presents in this order: major, minor, patch, while previously (and correctly), it presented: patch, minor, major.

Where is the correct order listed? semver lists them in this order:

const RELEASE_TYPES = [
  'major',
  'premajor',
  'minor',
  'preminor',
  'patch',
  'prepatch',
  'prerelease',
]

@sindresorhus
Copy link
Owner

I don't think there's any "correct" order, but for np, it should maintain the existing order. Also because having the most used type first (path) is useful.

Existing order:

❯ patch 	5.3.1
  minor 	5.4.0
  major 	6.0.0
  prepatch 	5.3.1-0
  preminor 	5.4.0-0
  premajor 	6.0.0-0
  prerelease 	5.3.1-0
  ──────────────
  Other (specify)

@sindresorhus
Copy link
Owner

One more thing, when running in non-node project, it crashes:

~/Downloads
❯ np
file:///opt/homebrew/lib/node_modules/np/source/version.js:4
const {packageJson: pkg} = await readPackageUp();
                    ^

TypeError: Cannot destructure property 'packageJson' of '(intermediate value)' as it is undefined.
    at file:///opt/homebrew/lib/node_modules/np/source/version.js:4:21

Node.js v18.17.1

@tommy-mitchell
Copy link
Collaborator Author

I've reverted the order and the backticks for the Version class.

when running in non-node project, it crashes

I don't think that's this branch of np, source/version.js doesn't have that line anymore.

@tommy-mitchell
Copy link
Collaborator Author

If I press Control+C (to exit) while it's running (for example, "Installing dependencies"), the text cursor is not restored.

I'm not sure what's causing this. I updated to the latest version exit-hook, maybe that?

@sindresorhus
Copy link
Owner

I don't think that's this branch of np, source/version.js doesn't have that line anymore.

Yeah. My mistake. Too much switching back and forth between the versions.

@sindresorhus
Copy link
Owner

I'm not sure what's causing this. I updated to the latest version exit-hook, maybe that?

That update simply renames an option though. So not sure why that would cause this.

The thing that is supposed to restore the cursor is called restore-cursor:

yarn why v1.22.19
[1/4] 🤔  Why do we have the module "restore-cursor"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "[email protected]"
info Reasons this module exists
   - "cli-cursor" depends on it
   - Hoisted from "cli-cursor#restore-cursor"
info Disk size without dependencies: "60KB"
info Disk size with unique dependencies: "104KB"
info Disk size with transitive dependencies: "124KB"
info Number of shared dependencies: 3
=> Found "inquirer#[email protected]"
info Reasons this module exists
   - "inquirer#cli-cursor" depends on it
   - Hoisted from "inquirer#cli-cursor#restore-cursor"
info Disk size without dependencies: "20KB"
info Disk size with unique dependencies: "64KB"
info Disk size with transitive dependencies: "84KB"
info Number of shared dependencies: 3
=> Found "listr-input#[email protected]"
info Reasons this module exists
   - "listr-input#inquirer#cli-cursor" depends on it
   - Hoisted from "listr-input#inquirer#cli-cursor#restore-cursor"
info Disk size without dependencies: "20KB"
info Disk size with unique dependencies: "64KB"
info Disk size with transitive dependencies: "84KB"
info Number of shared dependencies: 3
=> Found "ora#[email protected]"
info Reasons this module exists
   - "inquirer#ora#cli-cursor" depends on it
   - Hoisted from "inquirer#ora#cli-cursor#restore-cursor"
info Disk size without dependencies: "20KB"
info Disk size with unique dependencies: "64KB"
info Disk size with transitive dependencies: "84KB"
info Number of shared dependencies: 3
✨  Done in 0.15s.

@sindresorhus
Copy link
Owner

I'm gonna merge this as it will unblock other things, but I don't think we can release it until we figure out why the cursor is not restored as it will affect usage after using np.

@sindresorhus sindresorhus merged commit 1cb22e0 into sindresorhus:main Sep 2, 2023
3 checks passed
@sindresorhus
Copy link
Owner

Nice work on all this by the way. It's great to get more tests so we can make changes more confidently. 👍

@tommy-mitchell
Copy link
Collaborator Author

The thing that is supposed to restore the cursor is called restore-cursor

Seems it might've been updating inquirer, the newest version downgraded restore-cursor: SBoudrias/Inquirer.js@c51e33d#diff-b4c1d5e0907543f5ddd59a94a2ccdd1c3c790d0187d693c97e5d1bb86d55ab3b

@tommy-mitchell
Copy link
Collaborator Author

@sindresorhus is there a reason we're using ow for validation? It's only used in a couple of places, and it seems like we could just do the checks without adding another dependency:

np/source/util.js

Lines 63 to 75 in 1cb22e0

export const getTagVersionPrefix = pMemoize(async options => {
ow(options, ow.object.hasKeys('yarn'));
try {
const {stdout} = options.yarn
? await execa('yarn', ['config', 'get', 'version-tag-prefix'])
: await execa('npm', ['config', 'get', 'tag-version-prefix']);
return stdout;
} catch {
return 'v';
}
});

@sindresorhus
Copy link
Owner

is there a reason we're using ow for validation?

Feel free to remove it.

@sindresorhus
Copy link
Owner

@tommy-mitchell I'm still seeing the SemVer issue in the main branch:

Publish a new version of sindre-playground (current: 3.0.3)

? No commits found since previous release, continue? Yes
? Select SemVer increment or specify new version patch 	3.0.4

  ✔ Prerequisite check
  ✔ Git
  ✔ Installing dependencies using Yarn
  ✔ Running tests using Yarn
  ✔ Bumping version using Yarn
  ✔ Publishing package using Yarn
  ✔ Pushing tags
  ✖ Creating release draft on GitHub
    → New version 3.0.4 should either be one of patch, minor, major, prepatch, preminor, premajor, prerelease, or a valid SemVer version.

✖ Error: New version 3.0.4 should either be one of patch, minor, major, prepatch, preminor, premajor, prerelease, or a valid SemVer version.
    at Version.setFrom (file:///Users/sindresorhus/dev/oss/np/source/version.js:93:11)
    at releaseTaskHelper (file:///Users/sindresorhus/dev/oss/np/source/release-task-helper.js:9:30)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for utility files Add more tests
2 participants