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: add cluster.duplicate() method #926

Merged
merged 1 commit into from
Jul 14, 2019
Merged

feat: add cluster.duplicate() method #926

merged 1 commit into from
Jul 14, 2019

Conversation

SGrondin
Copy link
Contributor

Hi,

See #921. This PR adds the missing cluster.duplicate() method.

Notes:

  • I'm using this.startupNodes.slice(0) to ensure we pass a copy and not a reference. This matches the behavior of the options, which also get a copy thanks to Object.assign().
  • I'm not sure this new test I added is being executed when running npm test. I don't see it in the mocha output. It works as expected when I run npx mocha test/functional/cluster/duplicate.ts, could someone please validate?

@luin
Copy link
Collaborator

luin commented Jul 14, 2019

LGTM. The test should be executed since npm test simply calls mocha test/**/*.ts. "test/functional/cluster/duplicate.ts" matches the pattern. Does npx mocha test/**/*.ts execute the test?

@SGrondin
Copy link
Contributor Author

Wow, that's a quick response!

Unfortunately npx mocha test/**/*.ts fails to run any tests because of a compilation error in test/functional/scan_stream.ts. I'm guessing it's also why npm test isn't running the new test.

TSError: ⨯ Unable to compile TypeScript:
test/functional/scan_stream.ts:239:15 - error TS2339: Property 'sadd' does not exist on type 'Cluster'.
test/functional/scan_stream.ts:240:30 - error TS2339: Property 'sscanStream' does not exist on type 'Cluster'.

@luin
Copy link
Collaborator

luin commented Jul 14, 2019

I see. There are some issues related to type which will be fixed in the next major version. Just pushed a temp fix. Please merge the latest commit and try again.

@SGrondin
Copy link
Contributor Author

I've rebased on top of the latest commit and fixed the code formatting issues. The build is passing but I still don't see the new test. It should be under a describe grouped called #duplicate. All I see is the other duplicate test, for the base client.

@luin
Copy link
Collaborator

luin commented Jul 14, 2019

It seems that mocha ignores tests that contain TypeScript errors, which is the reason the newly added test wasn't executed, as well as some other tests. Didn't notice this issue before.

@luin
Copy link
Collaborator

luin commented Jul 14, 2019

@SGrondin Just did some fixes for the issues. Merge the latest commits will do the trick.

@SGrondin
Copy link
Contributor Author

Very nice. It ran 55 tests that were previously ignored.

@luin luin merged commit 8b150c2 into redis:master Jul 14, 2019
ioredis-robot pushed a commit that referenced this pull request Jul 14, 2019
# [4.12.0](v4.11.2...v4.12.0) (2019-07-14)

### Features

* **cluster:** add #duplicate() method ([#926](#926)) ([8b150c2](8b150c2))
@ioredis-robot
Copy link
Collaborator

🎉 This PR is included in version 4.12.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

3 participants