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

Refactor the message pass to the assert module in Test cluster setup master #16065

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@jbeatj

jbeatj commented Oct 7, 2017

In test/parallel/test-cluster-setup-master.js, a refactoring was done on the message pass to the assert module. Template literal was add in some case to give more information. Also, one assert was remove because the test was optimize by adding the "must call method" from common. Rather than incrementing a variable each time a worker go online and then assert that this number is equal to totalWorker. We specify that the callback after the worker get online are call totalWorker times with the "mustcall" method from common.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • Commit message follows [commit guidelines]
  • git fetch, rebase with upstream/master
Affected core subsystem(s)
  • None

Jean-Baptiste Brossard added some commits Oct 6, 2017

Jean-Baptiste Brossard
Refactor the message argument pass to the assert module.
- The assert "assert.ok(checks.workers, 'Not all workers went online');"
was remove, instead, mustCall was add around the callback that
is called after a worker get online. So you don't have to do
something like : "onlineWorker ++ , if onlineWorker == totalWorker".

- The assert that make sure if all the worker got their arguments got his message
refactor so that it include the number of worker that got
argument vs the expected number of argument.

- The assert that check if the setting object  have his properties fill got his message
refactor, it now print the content of the setting object.

@mscdex mscdex added the cluster label Oct 7, 2017

assert.ok(checks.workers, 'Not all workers went online');
assert.ok(checks.args, 'The arguments was noy send to the worker');
const argsMsg = `The arguments was not send for one or more worker.
There was ${correctInput} worker that receive

This comment has been minimized.

@joyeecheung

joyeecheung Oct 7, 2017

Member

The indentations will be preserved when they show up in assertions. Can you change this to using concatenations instead?

@jbeatj

This comment has been minimized.

jbeatj commented Oct 8, 2017

@joyeecheung I change the indentation for concatenation

@@ -49,7 +49,7 @@ if (cluster.isWorker) {
cluster.once('setup', function() {
checks.setupEvent = true;
const settings = cluster.settings;
settings = cluster.settings;

This comment has been minimized.

@BridgeAR

BridgeAR Oct 9, 2017

Member

Is this change actually needed at all?

This comment has been minimized.

@jbeatj

jbeatj Oct 9, 2017

it's to be able to stringify the "settings" variable inside the assert message.

This comment has been minimized.

@jbeatj

jbeatj Oct 9, 2017

@BridgeAR Hey, just wanna be sure about "This effects the test subsystem and the commit message do not follow the guidelines that are linked when opening a PR. It would be nice if this could be fixed." you want me to rewrite each commit message to make sure they follow the guide line ?!

This comment has been minimized.

@joyeecheung

joyeecheung Oct 10, 2017

Member

@jbeatj If you are comfortable with git, you can do a git rebase master -i and squash all the commits with one left as something like test: improve the assertion message

This comment has been minimized.

@joyeecheung

joyeecheung Oct 10, 2017

Member

@jbeatj Also you can leave this to whoever lands this and they would do that for you.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Oct 9, 2017

This effects the test subsystem and the commit message do not follow the guidelines that are linked when opening a PR. It would be nice if this could be fixed.

@joyeecheung

This comment has been minimized.

@joyeecheung

This comment has been minimized.

Member

joyeecheung commented Oct 15, 2017

Landed in e8a2438, thanks for the contribution!

joyeecheung added a commit that referenced this pull request Oct 15, 2017

test: refactor test-cluster-setup-master
- use mustCall instead of counters
- include totalWorkers and settings in the error messages

PR-URL: #16065
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

addaleax added a commit to ayojs/ayo that referenced this pull request Oct 15, 2017

test: refactor test-cluster-setup-master
- use mustCall instead of counters
- include totalWorkers and settings in the error messages

PR-URL: nodejs/node#16065
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

targos added a commit that referenced this pull request Oct 18, 2017

test: refactor test-cluster-setup-master
- use mustCall instead of counters
- include totalWorkers and settings in the error messages

PR-URL: #16065
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

targos added a commit that referenced this pull request Oct 18, 2017

test: refactor test-cluster-setup-master
- use mustCall instead of counters
- include totalWorkers and settings in the error messages

PR-URL: #16065
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

MylesBorins added a commit that referenced this pull request Nov 16, 2017

test: refactor test-cluster-setup-master
- use mustCall instead of counters
- include totalWorkers and settings in the error messages

PR-URL: #16065
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

@MylesBorins MylesBorins referenced this pull request Nov 21, 2017

Merged

v6.12.1 proposal #17180

MylesBorins added a commit that referenced this pull request Nov 21, 2017

test: refactor test-cluster-setup-master
- use mustCall instead of counters
- include totalWorkers and settings in the error messages

PR-URL: #16065
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

MylesBorins added a commit that referenced this pull request Nov 28, 2017

test: refactor test-cluster-setup-master
- use mustCall instead of counters
- include totalWorkers and settings in the error messages

PR-URL: #16065
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment