Update zookeeper to v3.4.12; migrate tests to TAP #14

bowrocker · 2020-02-27T15:53:15Z

Required submodule changes:

This change updates the version of Zookeeper to 3.4.12 in addition to updating the existing tests to run under TAP instead of nodeunit.

The tests are passing on a dev zone running zk 3.4.12 locally:


  🌈 SUMMARY RESULTS 🌈

Suites:   3 passed, 3 of 3 completed
Asserts:  93 passed, of 93
Time:     15s
--------------|----------|----------|----------|----------|-------------------|
File          |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
--------------|----------|----------|----------|----------|-------------------|
All files     |    59.67 |    54.72 |    51.85 |    59.61 |                   |
 index.js     |      100 |      100 |      100 |      100 |                   |
 recursion.js |     9.76 |        0 |        0 |     9.76 |... 82,383,384,386 |
 server.js    |    72.64 |    70.32 |       80 |    72.56 |... 18,619,622,625 |
 zk.js        |     89.6 |    68.18 |    86.96 |     89.6 |... 03,212,213,221 |
--------------|----------|----------|----------|----------|-------------------|

…ode-ufds update

trentm

Am I right that this change is just about a patch-level update of ZK? This isn't about the major/minor ZK version upgrades that (a) we eventually want to do and (b) will require us to test and consider upgrade issues.

trentm · 2020-02-27T18:05:28Z

Makefile

-BUILDIMAGE_PKGSRC =	sun-jre6-6.0.26 \
-			zookeeper-client-3.4.3 \
-			zookeeper-server-3.4.3
+BUILDIMAGE_PKGSRC =     openjdk8-1.8.232 zookeeper-3.4.12


nit: Say 3.4.12 here. Title says 3.4.11.

trentm · 2020-02-27T18:15:10Z

Makefile

+test: ensure-node-v6-or-greater-for-test-suite | $(TAP_EXEC)
+	@testFiles="$(shell ls test/integration/*.test.js | egrep "$(TEST_FILTER)")" && \
+	    test -z "$$testFiles" || \
+	    NODE_NDEBUG= $(TAP_EXEC) --timeout $(TEST_TIMEOUT_S) -j $(TEST_JOBS) -o ./test.tap $$testFiles


Could add these to .gitignore:

/.nyc_output /test.tap

Good catch.

trentm · 2020-02-27T18:16:10Z

Makefile

+deps $(TAP_EXEC): | $(REPO_DEPS) $(NPM_EXEC)
+	$(NPM_ENV) $(NPM) install
+
+.PHONY: ensure-node-v6-or-greater-for-test-suite


I'm not sure you need this change. Typically this is running using the specified (currently node v8) sdcnode build. This "ensure-node-v6-or-greater-for-test-suite" guard was originally added for node packages that don't specify a particular sdcnode. It doesn't hurt tho.

OK, I guess my inclination would be that if it's not strictly necessary then drop it.

trentm · 2020-02-27T18:16:47Z

Makefile

+.PHONY: ensure-node-v6-or-greater-for-test-suite
+ensure-node-v6-or-greater-for-test-suite: | $(TAP_EXEC)
+	@NODE_VER=$(shell node --version) && \
+	    ./node_modules/.bin/semver -r '>=6.x' $$NODE_VER >/dev/null || \


If you are keeping this guard target, then that semver binary is only there by luck. One should then add a "semver" dep to "devDependencies".

trentm · 2020-02-27T18:18:45Z

package.json

    "vasync": "2.1.0",
    "verror": "^1.10.0",
    "xtend": "1.0.3",
    "zkstream": "0.11.8"
  },
-  "engines": {
-    "node": ">=0.10"
-  },
  "devDependencies": {
    "expiring-lru-cache": "2.1.0",


It looks like nothing calls test/helper.js's createCache which is the only user of this dependency. Perhaps createCache and this dep could be dropped.

Please also consider cleaning up helper.js. It's before, after, and test are vestigial now that nodeunit has been dropped.

Ah, ok. I will see if I can drop it, and will do on the cleanup.

trentm · 2020-02-27T18:21:08Z

test/integration/database.test.js

-var core = require('../lib');
+var core = require('../../lib');
+
+var tap = require('tap');


style nit: Most (all?) our other tap-using test files tend to var test = require('tap').test; and just use test(...) in code later.

Note: Any "nit" ever from me in code review is fair game for you to ignore if your preference is otherwise.

Glad to get the input!

trentm · 2020-02-27T18:23:01Z

test/integration/database.test.js

 if (require.cache[__dirname + '/helper.js'])
        delete require.cache[__dirname + '/helper.js'];


Would you consider ditching this require.cache mucking? It is gross under-the-hood node hacking to ensure that helper.js is re-loaded for each test module. That was necessary when using nodeunit that ran all the test files in a single process. Node-tap runs each test file in a separate process so it should no longer be necessary.

trentm · 2020-02-27T18:27:24Z

Makefile

-NODE_PREBUILT_IMAGE =		fd2cc906-8938-11e3-beab-4359c665ac99
+NODE_PREBUILT_TAG =		zone64
+NODE_PREBUILT_VERSION :=	v8.17.0
+NODE_PREBUILT_IMAGE =		5417ab20-3156-11ea-8b19-2b66f5e7a439


You could consider taking this Makefile patch that will drop bothering to compile a local node v8 from source when running make all on non-SmartOS:

diff --git a/Makefile b/Makefile index e866fbc..d0cbe0d 100644 --- a/Makefile +++ b/Makefile @@ -41,7 +41,10 @@ ifeq ($(shell uname -s),SunOS) include ./deps/eng/tools/mk/Makefile.node_prebuilt.defs include ./deps/eng/tools/mk/Makefile.agent_prebuilt.defs else - include ./deps/eng/tools/mk/Makefile.node.defs + NPM=npm + NODE=node + NPM_EXEC=$(shell which npm) + NODE_EXEC=$(shell which node) endif include ./deps/eng/tools/mk/Makefile.node_modules.defs include ./deps/eng/tools/mk/Makefile.smf.defs

This is what other repos tend to do. There is no real value in our eng/tools/mk/Makefile.node.* makefiles.

trentm · 2020-02-27T18:29:37Z

package.json

  "devDependencies": {
    "expiring-lru-cache": "2.1.0",
-    "ejs": "2.7.2",
-    "nodeunit": "0.11.2"
+    "tap": "14.10.6"


One thing to check and discuss is how much bigger this has made the binder final images. That we tend to ship the tests and devDependencies currently in our images means that huge devDeps impact image size, which can slow down builds and upgrades. node-tap is huge, mainly because of its coverage support.

This isn't an answer, but see these two sections for some background: https://github.com/joyent/rfd/blob/master/rfd/0139/README.md#the-bad-with-node-tap and https://github.com/joyent/rfd/blob/master/rfd/0139/README.md#npm-install-smallertap

trentm · 2020-02-27T18:33:22Z

Also... make check fails. I believe that is because this is still using javascriptlint which can't handle the new "fat arrow" function syntax you are using. Options:

don't use the new fat arrow syntax for now
switch to eslint (and optionally prettier for code formatting as well),. rather than javascriptlint and jsstyle (BONUS POINTS)

If you consider the latter, you migth want that as a separate change to have the commits be cleaner. Also let me know and I can show exampels of other repos having made that change.

trentm · 2020-02-27T18:36:35Z

@bowrocker Is there a jira ticket for this work?

bowrocker · 2020-02-27T19:10:51Z

Am I right that this change is just about a patch-level update of ZK? This isn't about the major/minor ZK version upgrades that (a) we eventually want to do and (b) will require us to test and consider upgrade issues.

That's right, the idea is just to get us upgrade for now, then move to (a) and (b); this is from the ZK wiki:

Supported upgrade path: Existing stable 3.4.x versions should be supported, but in order to get the best results, it's strongly advised to upgrade to the latest stable 3.4 version beforehand.

…now.

timfoster

This is a bit of a drive-by review at the moment, but now that we're using modern node and npm, we should check in the package.lock file as generated from the sdcnode release that we're building with.

…rade fixes.

bowrocker · 2020-03-03T16:23:52Z

This is a bit of a drive-by review at the moment, but now that we're using modern node and npm, we should check in the package.lock file as generated from the sdcnode release that we're building with.

Good call, done.

bowrocker · 2020-03-12T12:37:54Z

Also... make check fails. I believe that is because this is still using javascriptlint which can't handle the new "fat arrow" function syntax you are using. Options:
1. don't use the new fat arrow syntax for now

2. switch to eslint (and optionally prettier for code formatting as well),. rather than javascriptlint and jsstyle  (BONUS POINTS)
If you consider the latter, you migth want that as a separate change to have the commits be cleaner. Also let me know and I can show exampels of other repos having made that change.

I went with the first option for now, but I will follow up with the latter once this PR is merged.

bowrocker and others added 8 commits February 21, 2020 18:31

Changes to Makefile for ZK upgrade to 3.4.11.

decdcfa

drop ejs dep and get devDependencies out of the way for now

f2935e4

some easy dep updates, TODO for artedi v2 for later, sdc-clients -> n…

506c697

…ode-ufds update

Package and Makefile updates for ZK 3.4.11 update.

8626853

Changes related to moving integration tests to TAP from nodeunit.

2a0dbb3

Whitespace in integration test files.

32dca86

Merge branch 'master' of github.com:joyent/binder into jra-zk-3.4.11

e655585

Remove debug console message.

67f1d69

bowrocker requested review from kellymclaughlin, timfoster and trentm February 27, 2020 15:56

trentm reviewed Feb 27, 2020

View reviewed changes

bowrocker changed the title ~~Update zookeeper to v3.4.11; migrate tests to TAP~~ Update zookeeper to v3.4.12; migrate tests to TAP Feb 27, 2020

bowrocker added 4 commits February 27, 2020 22:50

Review feedback on ZK 3.4.12 upgrade.

2d4f90b

Remove 'self' patching in the tests; remove 'fat arrow' notation for …

cf1fa72

…now.

Formatting fixes.

9b1a0eb

Further formatting fixes to tests.

b3b733f

timfoster suggested changes Mar 2, 2020

View reviewed changes

bowrocker added 6 commits March 2, 2020 16:43

Update Jenkinsfile to image_ver 19.4.0.

2902cf4

Update submodule versions in .gitmodules, fix some JS lint issues.

de91696

Update submodules javascriptlint and mname-balancer for binder ZK upg…

fcdf28b

…rade fixes.

Update to mname submodule

b741296

Replace ctfconvert make definitions.

1023a2a

Remove CTFTOOL definition from Makefile.

8b162b9

bowrocker requested a review from timfoster March 3, 2020 16:09

Checking package-lock.json as requested in the binder ZK review.

8bf78b6

Link to branch on zookeeper_common until it is merged.

d5d35ea

bowrocker mentioned this pull request Mar 12, 2020

Changes in support of ZK upgrade in binder/nameservice TritonDataCenter/zookeeper-common#1

Merged

This was referenced Mar 12, 2020

Fix formatting output for GCC update. jclulow/libcbuf#1

Open

Changes in support of ZK/GCC update to binder/nameservice TritonDataCenter/mname-balancer#3

Merged

Fix formatting output for newer GCC. bowrocker/libcbuf#1

Open

Update deps.

dd88681

This was referenced Mar 13, 2020

MANTA-3543 changes for GCC update on image 19.4.0 bowrocker/libcbuf#2

Closed

MANTA-3543 changes for GCC update on image 19.4.0 TritonDataCenter/libcbuf#1

Merged

bowrocker added 2 commits March 16, 2020 18:27

Updated mname-balance dependency.

803bd18

Update mname-balance submodule to lastest, after merges for ZK updated.

1e85513

timfoster removed request for kellymclaughlin and timfoster June 24, 2021 10:28

bahamat mentioned this pull request Feb 3, 2022

TRITON-349 Update binder to node.js v6 #18

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update zookeeper to v3.4.12; migrate tests to TAP #14

Update zookeeper to v3.4.12; migrate tests to TAP #14

bowrocker commented Feb 27, 2020 •

edited

Loading

trentm left a comment

trentm Feb 27, 2020

bowrocker Feb 27, 2020

trentm Feb 27, 2020

bowrocker Feb 27, 2020

trentm Feb 27, 2020

bowrocker Feb 27, 2020

trentm Feb 27, 2020

trentm Feb 27, 2020

bowrocker Feb 27, 2020

bowrocker Feb 27, 2020

trentm Feb 27, 2020

bowrocker Feb 27, 2020

bowrocker Feb 27, 2020

trentm Feb 27, 2020

bowrocker Feb 28, 2020

trentm Feb 27, 2020

bowrocker Feb 28, 2020

trentm Feb 27, 2020

trentm commented Feb 27, 2020

trentm commented Feb 27, 2020

bowrocker commented Feb 27, 2020 •

edited

Loading

timfoster left a comment

bowrocker commented Mar 3, 2020

bowrocker commented Mar 12, 2020

		if (require.cache[__dirname + '/helper.js'])
		delete require.cache[__dirname + '/helper.js'];

Update zookeeper to v3.4.12; migrate tests to TAP #14

Are you sure you want to change the base?

Update zookeeper to v3.4.12; migrate tests to TAP #14

Conversation

bowrocker commented Feb 27, 2020 • edited Loading

trentm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trentm commented Feb 27, 2020

trentm commented Feb 27, 2020

bowrocker commented Feb 27, 2020 • edited Loading

timfoster left a comment

Choose a reason for hiding this comment

bowrocker commented Mar 3, 2020

bowrocker commented Mar 12, 2020

bowrocker commented Feb 27, 2020 •

edited

Loading

bowrocker commented Feb 27, 2020 •

edited

Loading