Separate "logging" from "output" #1213

honzajavorek · 2019-01-17T17:27:26Z

🚀 Why this change?

Because it's needed if we want to revamp the reporters and make them better. See #1089

📝 Related issues and Pull Requests

🚧 To do

make two separate Winston logger instances, one for Dredd's "output" and the other for Dredd's "logging"
remove --level and --silent, replace it with --loglevel and --loglevel=silent, don't allow the option to control Dredd's "output"
remove all "logging" levels except of error, warning, debug, and silent
go through the codebase once again and carefully decide about every logger call whether it's "output" or "logging"
have existing tests passing
write unit tests for the --loglevel behavior (error, warning, warn, debug, silent, lower/upper case, default is warning)

✅ 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

test/integration/cli/configuration-cli-test.js

test/integration/cli/hookfiles-cli-test.js

lib/options.json

test/unit/Dredd-test.js

lib/CLI.js

lib/HooksWorkerClient.js

lib/reporters/ApiaryReporter.js

docs/usage-js.rst

A part of the effort to separate application logging from the reporters output. Enables #1089 and subsequently #765, supersedes #1099 BREAKING CHANGE: The --level option is no longer able to affect reporter output, it only affects application logging output.

A part of the effort to separate application logging from the reporters output. Addresses #1089, supersedes #1099, enables #765 BREAKING CHANGE: Instead of --level use --loglevel. The option is no longer able to affect reporter output, it only affects application logging output. Use --loglevel=silent instead of --silent, which is now removed. The --loglevel option now only accepts 'silent', 'error', 'warning', 'debug' as values.

BREAKING CHANGE: The --timestamp/-t option is no longer available. Instead, the --loglevel=debug automatically displays timestamps as well for the application logging entries.

BREAKING CHANGE: The --color option doesn't support string values ('false', 'true') anymore. To disable coloring, use --no-color instead. The -c short option has been removed.

This is a work around. The configuration and loggers setup should be refactored to work without side effects like this.

honzajavorek · 2019-02-01T21:58:57Z

@kylef I replied to your comments and added two commits to address some. Most of your comments lead to follow-up work, which I'm to document somewhere soon.

I'm defending my evil approach in #1213 (comment) and I'm interested in your opinion on this before I merge the PR.

And thanks very much for the review! 🙏

kylef · 2019-02-05T13:57:15Z

lib/configuration.js

+function deprecate(config = {}) {
+  if (config.options) {
+    if (config.options.c) {
+      throw new Error('DEPRECATED: Dredd does not support '


Since this is erroring, this is not deprecation. It's removal. Deprecation would be when it still works but is discouraged and potentially scheduled for removal in the future:

In several fields, deprecation is the discouragement of use of some terminology, feature, design, or practice, typically because it has been superseded or is no longer considered efficient or safe, without completely removing it or prohibiting its use. It can also imply that a feature, design, or practice will be removed or discontinued entirely in the future

From Wikipedia on Deprecation.

To fix, if you want to keep this behaviour I think you can rename it to now include "deprecated". Alternatively you could actually deprecate it and set config.options.equivilent option = foo etc

You're right. But while I could translate some of the options to the new ones, certain features are just gone. What word should I use to alert the user about the fact the feature is gone? If I don't do this, optimist silently passes even with extra options (yeah, facepalm).

I want Dredd to fail fast in case I know someone just tried to use a feature which is not in place anymore.

I think you can rename it to now include "deprecated"

Took me a moment to parse the sentence, but if you meant

I think you can rename it to not include "deprecated"

then I could do that, yes 😄

honzajavorek · 2019-02-05T14:20:57Z

The smoke test is failing as the dredd-example uses --level 😐 I'll remove it from there for the time of merging this PR and then I'll replace it with --loglevel.

honzajavorek · 2019-02-05T15:46:15Z

The smoke test failing is a valid alert that every setup using dredd.yml would fail the same way. While I could probably do that given it's a breaking version of Dredd, I don't think it's nice. I'm fine breaking someone's workflow in the terminal when they can correct it in a second, but to break Dredd setups all around the globe when upgrading, and requiring people to manually edit their dredd.yml files, that doesn't sound nice.

This all is because Dredd's configuration is so badly designed, but that's out of the scope of this PR, so I'll try to prepare a commit with a solution, albeit ugly. I'll properly deprecate all options which make the dredd-example build to fail.

honzajavorek · 2019-02-05T17:09:10Z

I added some ugly code to deal with the deprecations 🙈 At least it's tested. I'll think of a better way to do this in the future, but it probably won't get much better unless I decide to dive into #1101

honzajavorek · 2019-02-05T18:10:06Z

Here it is: ff55694

Added commits

honzajavorek · 2019-02-06T14:56:17Z

@kylef please re-review 🙏

kylef · 2019-02-06T22:43:37Z

test/unit/configuration-test.js

    it('the application logger is set to add timestamps', () => {
      assert.isTrue(logger.transports.console.timestamp);
    });
  });
 });
+
+
+describe('configuration._coerceRemovedOptions()', () => {


I don't like the way we are testing the specific unit for a few reasons (firstly we have to export this internal method for testing), but the biggest reason is it is perhaps error prone. Especially considering Dredd does not error out for invalid options. For example, if I invoke dredd with --foo or even a typo of a valid option, dredd will continue instead of erroring out:

$ dredd api-description.apib http://127.0.0.1:3000 --dry-run --foo info: Beginning Dredd testing... ... complete: Tests took 5ms

IMO this is a huge anti pattern because if a user typos or misenters an option it could result in undesired behaviour which could potentially be destructive.

I'll show you perhaps an example outside of Dredd scope:

$ rm -fr --dry-run / rm: illegal option -- - usage: rm [-f | -i] [-dPRrvW] file ... unlink file

Now, if --dry-run (perhaps I typoed --dri-run) doesn't exist the potentially destructive command did not proceed. An example closer to Dredd could be perhaps attempting to use Dredd CLI options to skip certain destructive methods. For example if I wanted to run Dredd on production and wanted to skip method DELETE for obvious reasons and I typoed the skip command I would hope that Dredd errors out due to invalid arguments instead of running a mis-understood command.

Anyway, my actual point (sorry got distracted on side-bug story/tangent 😀) is what if _coerceRemovedOptions created a invalid option that doesn't exist we would never know. Since the test is for the unintegrated unit (and also Dredd doesn't validate invalid options) we could have room for invalid code. Perhaps right now we can be 100% sure with careful review, but what about in a few years times when (future) you or someone else decides to remove an option --color because it is spelt wrong and they add --colour these tests would continue to work so they cannot protect against this type of bug. The tests don't actually know that the option was parsed correctly.

_coerceRemovedOptions could produce invalid options and our tests cannot catch them. If _coerceRemovedOptions created an option during conversion of -c to --color where value was wrong type, perhaps --color was refactored to take other values like --color red and not boolean, and this logic returns boolean (which is what its tests expected) the integration into test of the options parser is incorrect. Tests could continue to pass.

Anyway, tl;dr I am not sure these specific tests add much value over what the code does. They do not protect you against future refactoring which is the key reason I do TDD.

I have to admit I've written the tests ex post. They were useful in catching bugs in the function I wrote prior to them 😓

Your points are completely valid. I consider the whole file to be a brownfield. The fact Dredd is in general silent about mistyped options is terrible, but it is an existing behavior. It's solvable probably only by switching the CLI options parser and changing how the configuration is processed 😞

Perhaps _coerceRemovedOptions could have been checked also by an integration test, but ideally I remove all the checks after a period of time with a major release and introduce a better configuration management for Dredd (#1101).

tl;dr I completely agree, but I think fixing this is out of the scope of this PR. I added ugly things, I know it, and I hope I'll have time to improve the CLI later. And I hope if I won't have time to do it, future generations of people working on Dredd at least find these comments and won't hate me that much 🙏 😄

ApiaryBot · 2019-02-07T16:46:10Z

🎉 This PR is included in version 8.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

honzajavorek added the in progress label Jan 17, 2019

honzajavorek changed the title ~~Honzajavorek/separate logging from reporters~~ Separate application "logging" from application "output" Jan 17, 2019

honzajavorek changed the title ~~Separate application "logging" from application "output"~~ Separate "logging" from "output" Jan 17, 2019

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch 4 times, most recently from 548df20 to 6ab6169 Compare January 23, 2019 17:57

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch 3 times, most recently from e10bb3f to f2391eb Compare January 24, 2019 18:16

honzajavorek removed the in progress label Jan 24, 2019

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch from f2391eb to 6cbc6f2 Compare January 24, 2019 18:35

honzajavorek added in progress and removed in progress labels Jan 24, 2019

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch from 9108565 to b5c4a2c Compare January 24, 2019 19:12

honzajavorek requested a review from kylef January 24, 2019 19:13

honzajavorek mentioned this pull request Jan 31, 2019

Upgrade winston to 3.x #1225

Open

kylef approved these changes Feb 1, 2019

View reviewed changes

honzajavorek added 11 commits February 1, 2019 22:45

test: update tests to reflect the logging split

927046c

test: update tests to reflect the --level and --silent removal

79ff1ba

refactor: log only as error, warning, and debug

691c0a5

test: unit test processing of the --loglevel option

2f8f6b6

refactor: remove the --timestamp option

8eb265b

BREAKING CHANGE: The --timestamp/-t option is no longer available. Instead, the --loglevel=debug automatically displays timestamps as well for the application logging entries.

refactor: change behavior of --color

a5fe81b

BREAKING CHANGE: The --color option doesn't support string values ('false', 'true') anymore. To disable coloring, use --no-color instead. The -c short option has been removed.

test: make 'applyLoggingOptions()' tests more proof against side effects

7eb256b

This is a work around. The configuration and loggers setup should be refactored to work without side effects like this.

refactor: remove unnecessary logging

aea95c7

refactor: simplify 'setExitOrCallback()' logging

67b6b2e

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch from b5c4a2c to 67b6b2e Compare February 1, 2019 21:56

kylef approved these changes Feb 4, 2019

View reviewed changes

kylef previously requested changes Feb 5, 2019

View reviewed changes

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch from 072ce78 to bf5743a Compare February 5, 2019 14:19

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch 2 times, most recently from 8e11b58 to 3a2fc56 Compare February 5, 2019 17:03

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch from 3a2fc56 to 9ec199f Compare February 5, 2019 17:50

refactor: properly deprecate options

ff55694

honzajavorek force-pushed the honzajavorek/separate-logging-from-reporters branch from 9ec199f to ff55694 Compare February 5, 2019 18:02

kylef reviewed Feb 6, 2019

View reviewed changes

kylef approved these changes Feb 7, 2019

View reviewed changes

refactor: modify the options in place

c8cf96f

honzajavorek merged commit 589c7eb into master Feb 7, 2019

honzajavorek deleted the honzajavorek/separate-logging-from-reporters branch February 7, 2019 16:38

ApiaryBot added the released label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separate "logging" from "output" #1213

Separate "logging" from "output" #1213

honzajavorek commented Jan 17, 2019 •

edited

Loading

honzajavorek commented Feb 1, 2019 •

edited

Loading

kylef Feb 5, 2019

kylef Feb 5, 2019

honzajavorek Feb 5, 2019

honzajavorek Feb 5, 2019

honzajavorek Feb 5, 2019

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 6, 2019

kylef Feb 6, 2019 •

edited

Loading

kylef Feb 6, 2019 •

edited

Loading

honzajavorek Feb 7, 2019

honzajavorek Feb 7, 2019

ApiaryBot commented Feb 7, 2019

Separate "logging" from "output" #1213

Separate "logging" from "output" #1213

Conversation

honzajavorek commented Jan 17, 2019 • edited Loading

🚀 Why this change?

📝 Related issues and Pull Requests

🚧 To do

✅ What didn't I forget?

honzajavorek commented Feb 1, 2019 • edited Loading

kylef Feb 5, 2019

Choose a reason for hiding this comment

kylef Feb 5, 2019

Choose a reason for hiding this comment

honzajavorek Feb 5, 2019

Choose a reason for hiding this comment

honzajavorek Feb 5, 2019

Choose a reason for hiding this comment

honzajavorek Feb 5, 2019

Choose a reason for hiding this comment

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 5, 2019

honzajavorek commented Feb 6, 2019

kylef Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

kylef Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

honzajavorek Feb 7, 2019

Choose a reason for hiding this comment

honzajavorek Feb 7, 2019

Choose a reason for hiding this comment

ApiaryBot commented Feb 7, 2019

honzajavorek commented Jan 17, 2019 •

edited

Loading

honzajavorek commented Feb 1, 2019 •

edited

Loading

kylef Feb 6, 2019 •

edited

Loading

kylef Feb 6, 2019 •

edited

Loading