Refactor Dredd.js, improve integration tests coverage, drop Node 6 support #1317

honzajavorek · 2019-04-04T13:20:01Z

🚀 Why this change?

I didn't like most of the Dredd.js file. This should streamline the pipeline from API description locations to parse results and transactions. The refactoring prepares ground for #1191. With #1275 it could be further refactored and streamlined.

I still feel like I did this, but I hope it is still improvement in gradually isolating stuff.

📝 Related issues and Pull Requests

Supersedes Refactor Dredd.js #1289
Closes Reorganization and cleanup of Dredd.js Integration Tests #1288
Prepares ground for handleRuntimeProblems & blueprint utils should be merged, renamed, better tested #1191

✅ What didn't I forget?

To write docs
To write tests
To put Conventional Changelog prefixes in front of all my commits and run npm run lint

honzajavorek · 2019-04-09T13:20:39Z

lib/Dredd.js

@@ -1,21 +1,89 @@
 const async = require('async');


The code of this file still seems ugly to me at many places and I blame mainly the asynchronous parts. I hope with async/await/promises it could get even more readable and unit-testable in the future.

honzajavorek · 2019-04-09T13:21:00Z

lib/Dredd.js

 const resolveModule = require('./resolveModule');
 const logger = require('./logger');
 const TransactionRunner = require('./TransactionRunner');
 const { applyConfiguration } = require('./configuration');


-const FILE_DOWNLOAD_TIMEOUT = 5000;
+function prefixErrors(decoratedCallback, prefix) {


I couldn't come up with a nicer way how to prefix errors with context than decorating the callback like this. I might be just missing a good idea or a best practise. But it's gonna disappear with async/await/promises anyway.

honzajavorek · 2019-04-09T13:21:22Z

lib/Dredd.js

+}
+
+
+function readLocations(locations, options, callback) {


I feel like readLocations() and parseContent() might get a lot slimmer or even redundant with Promise.all() or Promise.map(), but I decided to stay with callbacks until Dredd drops Node 6 support.

honzajavorek · 2019-04-09T13:23:00Z

lib/Dredd.js

-    }
-
-    this.logger.debug('Resolving API descriptions locations');
+  prepareAPIdescriptions(callback) {


The prepareAPIdescriptions() method isn't a real method of a real object as the Dredd class doesn't need to be a class in the first place. I created it just to make run() shorter. It roughly contains the pipeline turning API description locations into API descriptions, then parsing them, and then compiling them into transactions. Once we have async/await/promises and the code is less tied with 'this.configuration', it might make sense to refactor this further.

honzajavorek · 2019-04-09T13:23:45Z

lib/Dredd.js

-Is the provided path correct?
-`);
-        return callback(err);
+  run(callback) {


The Dredd class represents an object with just a single (asynchronous...) method, run(). That's an antipattern and a clear indication it doesn't need to be a class at all. It is interconnected by references with reporters though and it's Dredd's public JavaScript API, so I'm leaving it as a class for now. Also for now ignoring the fact that it calls the callback with both error and the stats (fixing it would be a breaking change). In the future I can imagine even something like const stats = await dredd(config);

honzajavorek · 2019-04-09T13:24:09Z

lib/Dredd.js

-          .map(transaction => (Object.assign({ apiDescriptionMediaType: compileResult.mediaType }, transaction)));
-        next();
+    this.logger.debug('Configuring reporters');
+    configureReporters(this.configuration, this.stats, this.transactionRunner);


The configureReporters() is a rabbit hole of side effects. Leaving it as it is for now.

honzajavorek · 2019-04-09T13:24:29Z

lib/Dredd.js

+      this.configuration.apiDescriptions = apiDescriptions;
+
+      // TODO https://github.com/apiaryio/dredd/issues/1191
+      const annotationsError = handleRuntimeProblems(apiDescriptions, this.logger);


I want to get rid of handleRuntimeProblems() as the next step after this PR, finally unblocking upgrading Dredd Transactions.

lib/Dredd.js

artem-zakharchenko · 2019-04-12T11:33:33Z

lib/Dredd.js

+      try {
+        return compile(mediaType, apiElements, location);
+      } catch (error) {
+        error.message = (


If you separate the decoration logic from the prefixError so it doesn't include a callback dispatch, you can reuse it here.

const decorateError = (error, prefix) => { error.message = `${prefix}: ${error.message}` return error } const prefixError = (...) => { // ... decodateError(...) // handle callbacks and stuff } // compileTransactions // ... } catch (error) { decorateError(error, 'Unable to compile ...') }

artem-zakharchenko · 2019-04-12T11:39:43Z

lib/Dredd.js

-      this.files = resolveLocations(this.configuration.custom.cwd, this.configuration.options.path);
-    } catch (err) {
-      callback(err, this.stats);
+      locations = resolveLocations(this.configuration.custom.cwd, this.configuration.options.path);


We need to time the merges of this and configuration pull request. I vote for merging configuration updates first, since they didn't touch any actual logic in Dredd, or other files, but simply removed the .options. nesting from internal usage. It would be much more simple to remove options nesting than ensure the logic introduced by your pull request is not lost.

I agree, let's merge yours first. Also, your PR is semantically unreleasable, but this is a breaking change, so if yours goes first, my will release both.

lib/TransactionRunner.js

artem-zakharchenko · 2019-04-12T11:46:19Z

lib/TransactionRunner.js

      const status = parseInt(response.status, 10);
      if ((status < 200) || (status >= 300)) {
        skip = true;
      }
    }
-    delete transaction.apiDescriptionMediaType;
+    delete transaction.apiDescription;


Is deleting the whole description intended?

Yes. It is currently passed along only for the purpose of checking the media type above. I have future plans with it, so I keep it as an object, but currently it is not further used anywhere else, so I'm removing it immediately after it's used, to prevent it to leak into hooks etc.

lib/isURL.js

artem-zakharchenko · 2019-04-12T11:54:27Z

lib/resolveLocations.js

+    // flattens the array of arrays
+    .reduce((flatArray, array) => flatArray.concat(array), [])
+    // removes duplicates
+    .filter((location, index, array) => !array.slice(0, index).includes(location));


Can we use new Set() to achieve the same output?

Sure 🤦‍♂️ That's even how resolvePaths() does it...

test/integration/js-interface-test.js

artem-zakharchenko · 2019-04-12T11:58:48Z

test/integration/js-interface-test.js

+      assert.instanceOf(dreddRuntimeInfo.err, Error);
+    });
+    it('passes expected stats to the callback', () => {
+      assert.hasAllKeys(dreddRuntimeInfo.stats, [


Can we separate the list of Dredd's stats keys into a enum?

test/unit/isURL-test.js

honzajavorek · 2019-04-12T15:40:33Z

@artem-zakharchenko Thanks for the review! 🙏 I'll add a few commits to address your comments and then, if you approve, let's block this PR until yours gets merged.

Close #1288

BREAKING CHANGE: The 'fileBasedReporters' property has been in the 'stats' object by accident and now is removed. Apparently it has been there for a while, as it got listed in #599, the only documentation of the 'stats' object to this date.

honzajavorek · 2019-04-30T16:29:39Z

I rebased this against master. As this is going to be a breaking change again because of 59702d4, I'll add a commit dropping Node 6 support, to lower the frequency of major Dredd releases per day 📈

Allowing for #1275 BREAKING CHANGE: Dredd does not support Node 6 anymore. Node 6 has a scheduled maintenance end on April 2019.

artem-zakharchenko

Looks good to me. Thank you for the changes.

ApiaryBot · 2019-05-02T13:54:49Z

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

honzajavorek force-pushed the honzajavorek/refactor-dredd-js branch 3 times, most recently from eba7565 to b67c6b5 Compare April 9, 2019 13:19

honzajavorek commented Apr 9, 2019

View reviewed changes

honzajavorek marked this pull request as ready for review April 9, 2019 13:29

honzajavorek requested review from kylef and artem-zakharchenko and removed request for kylef April 9, 2019 13:29

honzajavorek added this to In progress in SJS Apr 11, 2019

artem-zakharchenko reviewed Apr 12, 2019

View reviewed changes

honzajavorek added the in progress label Apr 12, 2019

honzajavorek added 7 commits April 30, 2019 10:57

test: revamp integration tests for Dredd's JS interface

ec07593

Close #1288

refactor: change how 'resolveLocations()' works

562f7dc

refactor: revamp the pipeline from API desc. locations to transactions

822b259

refactor: reuse error prefixing

f701cff

refactor: use Set to make the list unique

855f78b

test: reuse the list of expected stats keys

d3d77c3

honzajavorek force-pushed the honzajavorek/refactor-dredd-js branch from 0ac9b7c to d3d77c3 Compare April 30, 2019 16:22

honzajavorek changed the title ~~Refactor Dredd.js, improve integration tests coverage~~ Refactor Dredd.js, improve integration tests coverage, drop Node 6 support Apr 30, 2019

chore: drop Node 6 support

36c3ca7

Allowing for #1275 BREAKING CHANGE: Dredd does not support Node 6 anymore. Node 6 has a scheduled maintenance end on April 2019.

honzajavorek mentioned this pull request Apr 30, 2019

Update untildify to the latest version 🚀 #1342

Merged

honzajavorek mentioned this pull request Apr 30, 2019

Update make-dir to the latest version 🚀 #1300

Merged

artem-zakharchenko approved these changes May 2, 2019

View reviewed changes

honzajavorek merged commit 429e709 into master May 2, 2019

SJS automation moved this from In progress to Done May 2, 2019

honzajavorek deleted the honzajavorek/refactor-dredd-js branch May 2, 2019 13:47

ApiaryBot added the released label May 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Dredd.js, improve integration tests coverage, drop Node 6 support #1317

Refactor Dredd.js, improve integration tests coverage, drop Node 6 support #1317

honzajavorek commented Apr 4, 2019 •

edited

honzajavorek Apr 9, 2019

honzajavorek Apr 9, 2019

honzajavorek Apr 9, 2019

honzajavorek Apr 9, 2019

honzajavorek Apr 9, 2019

honzajavorek Apr 9, 2019

honzajavorek Apr 9, 2019

artem-zakharchenko Apr 12, 2019

artem-zakharchenko Apr 12, 2019

honzajavorek Apr 12, 2019

artem-zakharchenko Apr 12, 2019

honzajavorek Apr 12, 2019

artem-zakharchenko Apr 12, 2019

honzajavorek Apr 12, 2019

artem-zakharchenko Apr 12, 2019

honzajavorek commented Apr 12, 2019

honzajavorek commented Apr 30, 2019

artem-zakharchenko left a comment

ApiaryBot commented May 2, 2019

Refactor Dredd.js, improve integration tests coverage, drop Node 6 support #1317

Refactor Dredd.js, improve integration tests coverage, drop Node 6 support #1317

Conversation

honzajavorek commented Apr 4, 2019 • edited

🚀 Why this change?

📝 Related issues and Pull Requests

✅ What didn't I forget?

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

honzajavorek commented Apr 12, 2019

honzajavorek commented Apr 30, 2019

artem-zakharchenko left a comment

Choose a reason for hiding this comment

ApiaryBot commented May 2, 2019

honzajavorek commented Apr 4, 2019 •

edited