Move to a modern testing infrastructure #304

ishitatsuyuki · 2018-03-21T09:21:46Z

Migrated to Jest, promisified things.

Coder206 · 2018-03-22T01:22:53Z

test/api.js

@@ -0,0 +1,173 @@
+/* eslint-disable node/no-unsupported-features */
+// Copyright © 2017 Jan Keromnes. All rights reserved.


@ishitatsuyuki Nit: year++ (2018). Also, feel welcome to add your name/identifier here if you want.
@jankeromnes Do you think we should generalize this to the Janitor team? (ie: Copyright (c) 2018 Team Janitor. All rights reserved.)

ishitatsuyuki · 2018-03-22T11:52:26Z

One thing I'd like to mention is that I'm now just throwing unhandled promise rejection around for the boot process. I know that this should be fixed but I'm not sure which style is best.

jankeromnes

Wow, thanks a lot for singlehandedly refactoring lib/boot.js, lib/docker.js, lib/machines.js and lib/certificates.js! I'm impressed.

I'm the future, please separate such refactoring into separate pull requests, but I think this was much needed and modernizes our code base a lot. So, again, thanks! 👍

jankeromnes · 2018-04-03T20:33:34Z

api/hosts-api.js

-    'List all files that were modified (Kind: 0), added (1) or deleted (2) ' +
-    'in a given Docker container.',
+  'List all files that were modified (Kind: 0), added (1) or deleted (2) ' +
+  'in a given Docker container.',



Nit: Please re-indent these two lines.

jankeromnes · 2018-04-03T20:36:16Z

app.js

+  boot.forwardHttp(),
+  boot.ensureHttpsCertificates(),
+  boot.ensureDockerTlsCertificates()
+]).then(() => {


Nit: Please .catch(error => { log('[fail] could not start app', error); }) at the bottom.

jankeromnes · 2018-04-03T20:41:09Z

join.js

+  boot.ensureDockerTlsCertificates(),
+  boot.verifyJanitorOAuth2Access()
+])
+  .then(() => boot.registerDockerClient())


Nit: I don't really like the line break before this line, but I'm not sure what coding style would look better. @nt1m would you have any suggestions here?

Maybe remove the line break like so: ]).then(?

jankeromnes · 2018-04-03T20:43:36Z

lib/boot.js

@@ -3,6 +3,10 @@

 const fs = require('fs');
 const http = require('http');
+const {promisify} = require('util');


Nit: Please add spaces within the brackets around promisify. Also, if ESLint doesn't complain about this, please look into enforcing this rule.

jankeromnes · 2018-04-03T20:46:41Z

lib/boot.js

-    next();
-  });
+  const listen = promisify(forwarder.listen);
+  return listen.call(forwarder, ports.http);


Nit: You lost a log along the way. Please add it back in a .then(() => {}) here.

jankeromnes · 2018-04-03T20:50:18Z

lib/boot.js

-      https.ca = [ certificate.ca ];
+  return certificates.createHTTPSCertificate(parameters)
+    .then(({certificate, accountKey}) => {
+      https.ca = [certificate.ca];


Nit: Please add back spaces within brackets and parents, or please champion a new coding style to be used consistently in the entire codebase (and please enforce whatever option you choose via eslint).

jankeromnes · 2018-04-03T20:53:49Z

lib/boot.js

    }
+    throw error;


Nit: Please only throw when the error isn't a ENOENT (otherwise you'll never be able to successfully start a server for the first time). Also, in general, please never change the code behavior while refactoring.

jankeromnes · 2018-04-03T20:57:43Z

lib/boot.js

@@ -142,42 +120,41 @@ exports.ensureDockerTlsCertificates = function (next) {
  let serverValid = false;

  if (!caValid) {
-    resetAllCertificates();
+    await resetAllCertificates();


Nit: These function definitions inside a function were written because there was no async/await at the time. I think now you could just check for what's missing with if statements, and just await asynchronous stuff when necessary (removing functions like resetAllCertificates).

It looks like the logic is a bit complex here; please forgive me to keep the original structure.

Ok sure, this can be done in a follow-up.

jankeromnes · 2018-04-03T20:58:01Z

lib/boot.js

    return;
  }

  clientValid = certificates.isValid({
-    ca: [ ca.crt ],
+    ca: [ca.crt],


Nit: Please don't.

jankeromnes

Another fresh batch of comments!

Thanks again for all the hard work! But unfortunately, refactoring large portions of the code base in the same pull request as another change makes for very long, painful and numerous review cycles.

In the future, please refactor files one by one, in dedicated pull requests. This should make the whole process faster and easier for everyone. 😄

jankeromnes · 2018-04-14T08:36:38Z

join.js

+  boot.verifyJanitorOAuth2Access()
+])
+  .then(() => boot.registerDockerClient())
+  .catch((err) => log('[fail] could not join cluster', err))


Nit: No need for parentheses around err. Also, please use the complete word error (it doesn't cost much, so we tend to use full words in this codebase).

I'll add an eslint rule for this :)

jankeromnes · 2018-04-14T08:38:08Z

join.js

+  boot.ensureDockerTlsCertificates(),
+  boot.verifyJanitorOAuth2Access()
+])
+  .then(() => boot.registerDockerClient())


Maybe remove the line break like so: ]).then(?

jankeromnes · 2018-04-14T08:41:12Z

.eslintrc.js

@@ -19,6 +19,8 @@ module.exports = {
    "camelcase": "off",
    "no-var": "error",
    "prefer-const": "error",
+    "array-bracket-spacing": ["error", "always", {"objectsInArrays": false}],
+    "object-curly-spacing": ["error", "always"],


Thanks! But nit: Please respect this style in .eslintrc.js itself as well (by adding spaces inside your []).

jankeromnes · 2018-04-14T10:15:00Z

app.js

+  boot.ensureHttpsCertificates(),
+  boot.ensureDockerTlsCertificates()
+])
+  .catch(error => { log('[fail] could not start app', error); })


Nit: Please remove the above line break like so: ]).catch(error => {.

jankeromnes · 2018-04-14T10:48:16Z

lib/boot.js

@@ -3,6 +3,10 @@

 const fs = require('fs');
 const http = require('http');
+const { promisify } = require('util');
+const chmod = promisify(fs.chmod);


Nit: Please separate imports of different types into different blocks (i.e. here, please separate the require() and the promisify() blocks by an empty line).

jankeromnes · 2018-04-14T11:12:54Z

lib/machines.js

-      db.save();
-    });
+  // Quickly authorize the user's public SSH keys to access this container.
+  deploySSHAuthorizedKeys(user, machine).then(() => {


Nit: Please rebase your pull request. This function no longer exists.

jankeromnes · 2018-04-14T11:13:37Z

lib/machines.js

-      return;
-    }
-
+  docker.removeContainer({ host, container: containerId }, () => {


Nit: Shouldn't you use .then and .catch here, instead of sending 3 arguments to docker.removeContainer() ?

jankeromnes · 2018-04-14T11:16:04Z

lib/machines.js

    // Recycle the machine's name and ports.
    machine.status = 'new';
    machine.docker.container = '';
    db.save();

    log('destroy', containerId.slice(0, 16), 'success');
    callback();
+  }, (error) => {


Nit: Please use catch and remove the extra parentheses.

jankeromnes · 2018-04-14T11:17:04Z

lib/machines.js

  });
 };

 // Reset a user machine's list of authorized SSH public keys.
-function deploySSHAuthorizedKeys (user, machine, callback) {
+async function deploySSHAuthorizedKeys (user, machine) {


Nit: Please rebase. This function no longer exists.

jankeromnes · 2018-04-14T11:21:18Z

lib/docker.js

+        const buffer = Buffer.concat(chunks);
+        const container = docker.getContainer(containerId);
+
+        resolve(await container.putArchive(buffer, { path }));


Nit: No need to mark this callback as async, because you don't need to await here (you can just resolve the Promise with another Promise and it should just work, also this probably removes the need for the try/catch).

Use Jest as test framework, and restructured ESLint config files so it applies to each directory separately.

Coder206 reviewed Mar 22, 2018

View reviewed changes

ishitatsuyuki force-pushed the test branch 2 times, most recently from 472961a to e1a7a60 Compare March 22, 2018 06:40

jankeromnes added the Server label Apr 3, 2018

jankeromnes reviewed Apr 3, 2018

View reviewed changes

ishitatsuyuki force-pushed the test branch 2 times, most recently from 868e66e to 0136bf6 Compare April 4, 2018 02:34

jankeromnes reviewed Apr 14, 2018

View reviewed changes

ishitatsuyuki added 3 commits April 17, 2018 15:24

Promisify boot

e7ffdd3

Promisify docker

c2a1bcf

Move to a modern testing infrastructure

5e3400b

Use Jest as test framework, and restructured ESLint config files so it applies to each directory separately.

ishitatsuyuki force-pushed the test branch from 0136bf6 to 5e3400b Compare April 17, 2018 06:32

async-ify

30a2251

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to a modern testing infrastructure #304

Move to a modern testing infrastructure #304

ishitatsuyuki commented Mar 21, 2018 •

edited

Loading

Coder206 Mar 22, 2018

ishitatsuyuki commented Mar 22, 2018

jankeromnes left a comment

jankeromnes Apr 3, 2018

jankeromnes Apr 3, 2018

jankeromnes Apr 3, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 3, 2018

jankeromnes Apr 3, 2018

jankeromnes Apr 3, 2018

jankeromnes Apr 3, 2018

jankeromnes Apr 3, 2018

ishitatsuyuki Apr 4, 2018

jankeromnes Apr 6, 2018

jankeromnes Apr 3, 2018

jankeromnes left a comment

jankeromnes Apr 14, 2018

ishitatsuyuki Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

jankeromnes Apr 14, 2018

		@@ -0,0 +1,173 @@
		/* eslint-disable node/no-unsupported-features */
		// Copyright © 2017 Jan Keromnes. All rights reserved.

Move to a modern testing infrastructure #304

Are you sure you want to change the base?

Move to a modern testing infrastructure #304

Conversation

ishitatsuyuki commented Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

ishitatsuyuki commented Mar 22, 2018

jankeromnes 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

jankeromnes 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

ishitatsuyuki commented Mar 21, 2018 •

edited

Loading