[New] use `process.env.NODE_TAPE_OBJECT_PRINT_DEPTH` for the default object print depth #420

inadarei · 2018-01-02T22:16:32Z

Issue #419

ljharb

This contains a doc change only; there's no functionality change included?

ljharb · 2018-01-02T22:17:36Z

readme.markdown

@@ -15,10 +15,10 @@ var test = require('tape');

 test('timing test', function (t) {
    t.plan(2);
-


please revert unrelated whitespace changes.

Ha, yeah, I was just looking into how to stop VS Code from adding those :)

…object print depth.

ljharb · 2018-01-03T01:58:14Z

lib/results.js

+            if (process.env[depthEnvVar].toLowerCase() === 'infinity') {
+                res.objectPrintDepth = Infinity;
+            } else {
+                res.objectPrintDepth = parseInt(process.env[envVarDepth]);


this should have an explicit radix (, 10) for older envs

ljharb · 2018-01-03T05:46:21Z

readme.markdown

@@ -148,7 +148,8 @@ Available `opts` options are:
 - opts.timeout = 500. Set a timeout for the test, after which it will fail.
  See test.timeoutAfter.
 - opts.objectPrintDepth = 5. Configure max depth of expected / actual object
-  printing.
+  printing. Environmental variable `NODE_TAPE_OBJECT_PRINT_DEPTH` can set the 
+  desired depth for all tests and overrides any locally-set values.


should this override? or should the env var merely set the default, so that local intentional overrides still take precedence?

Was wondering the same, since good arguments can be made for both approaches. Finally decided to go the current route because:

seems more compatible with 12-Factor Apps guidance

env vars overriding is how https://www.npmjs.com/package/config works

I've no interest in complying with "12-factor apps" by itself; if it isn't the best thing for the project, then it shouldn't be done.

In general, i would agree that an env var should always override an equivalent setting - but there is no equivalent for "set every object print depth" in tape. Currently, every one is per-test, and per-test overrides imo should always take precedence.

OK, no problem. That makes sense. I was just explaining the logic behind the initial decision, but not doing it that way also makes sense. I will make the change as you requested.

ljharb · 2018-01-04T06:43:07Z

lib/test.js

    this._plan = undefined;
    this._cb = args.cb;
    this._progeny = [];
    this._ok = true;
+    var depthEnvVar = 'NODE_TAPE_OBJECT_PRINT_DEPTH';
+    if (args.opts.objectPrintDepth) {
+        this._objectPrintDepth = args.opts.objectPrintDepth


this line is missing a semicolon

ljharb · 2018-01-04T06:43:36Z

lib/test.js

+    var depthEnvVar = 'NODE_TAPE_OBJECT_PRINT_DEPTH';
+    if (args.opts.objectPrintDepth) {
+        this._objectPrintDepth = args.opts.objectPrintDepth
+    } else if (process.env[depthEnvVar]) {


instead of storing the key name in a variable, and extracting it multiple times, can we just extract the value into a variable right off the bat?

ljharb · 2018-01-04T06:47:03Z

lib/test.js

+    if (args.opts.objectPrintDepth) {
+        this._objectPrintDepth = args.opts.objectPrintDepth
+    } else if (process.env[depthEnvVar]) {
+        if (process.env[depthEnvVar].toLowerCase() === 'infinity') {


we should try to be robust against delete String.prototype.toLowerCase && delete RegExp.prototype.test (by caching the prototype method at module level)

Sorry, I am not sure I understand what you are proposing.

ljharb · 2018-01-04T06:51:49Z

lib/test.js

+        if (process.env[depthEnvVar].toLowerCase() === 'infinity') {
+            this._objectPrintDepth = Infinity;
+        } else {
+            this._objectPrintDepth = parseInt(process.env[depthEnvVar], 10);


hm, i'm not actually sure about the parseInt here. tape doesn't seem to do any typechecking on the objectPrintDepth option - ie, you could pass a non-number or a non-integer and it'd just coerce to a number later. Maybe we should do the same here?

ljharb · 2018-01-05T06:57:23Z

lib/test.js

@@ -56,14 +56,14 @@ function Test (name_, opts_, cb_) {
    this._cb = args.cb;
    this._progeny = [];
    this._ok = true;
-    var depthEnvVar = 'NODE_TAPE_OBJECT_PRINT_DEPTH';
+    var depthEnvVar = process.env['NODE_TAPE_OBJECT_PRINT_DEPTH']


please always use dot access instead of bracket access in javascript wherever possible.

ljharb · 2018-01-05T22:05:53Z

lib/test.js

+    if (args.opts.objectPrintDepth) {
+        this._objectPrintDepth = args.opts.objectPrintDepth;
+    } else if (depthEnvVar) {
+        if (depthEnvVar.toLowerCase() === 'infinity') {


To clarify: we should have var toLowerCase = String.prototype.toLowerCase; near the top of the file, and then here, do toLowerCase.call(depthEnvVar) instead of assuming that nobody's done delete String.prototype.toLowerCase inside their tests.

ljharb

LGTM

inadarei · 2018-01-10T18:39:18Z

Thanks for the approval and assitance in getting there. What are the next steps for a merge?

- [New] use `process.env.NODE_TAPE_OBJECT_PRINT_DEPTH` for the default object print depth (#420) - [New] Add "onFailure" listener to test harness (#408) - [Fix] fix stack where actual is falsy (#400) - [Fix] normalize path separators in stacks (#402) - [Fix] fix line numbers in stack traces from inside anon functions (#387) - [Fix] Fix dirname in stack traces (#388) - [Robustness] Use local reference for clearTimeout global (#385) - [Deps] update `function-bind`

ljharb requested changes Jan 2, 2018

View reviewed changes

[New] use process.env.NODE_TAPE_OBJECT_PRINT_DEPTH for the default …

17276d7

…object print depth.

inadarei mentioned this pull request Jan 3, 2018

Allow Setting opts.objectPrintDepth Globally #418

Closed

ljharb requested changes Jan 3, 2018

View reviewed changes

ljharb reviewed Jan 4, 2018

View reviewed changes

ljharb reviewed Jan 5, 2018

View reviewed changes

ljharb approved these changes Jan 7, 2018

View reviewed changes

ljharb added the enhancement label Jan 7, 2018

ljharb changed the title ~~Allow Setting opts.objectPrintDepth Globally~~ [New] use process.env.NODE_TAPE_OBJECT_PRINT_DEPTH for the default object print depth Jan 15, 2018

ljharb merged commit f26375c into tape-testing:master Jan 15, 2018

ljharb mentioned this pull request Jan 15, 2018

Better Formatting of Large JSON Objects in expected/actual Reports #419

Closed

inadarei deleted the global-depth-env-var branch January 15, 2018 20:56

inadarei mentioned this pull request Jan 21, 2018

Better formatting for large expected/actual objects #417

Closed

wooorm mentioned this pull request Dec 13, 2020

Shorter NODE_TAPE_OBJECT_PRINT_DEPTH env variable #534

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[New] use `process.env.NODE_TAPE_OBJECT_PRINT_DEPTH` for the default object print depth #420

[New] use `process.env.NODE_TAPE_OBJECT_PRINT_DEPTH` for the default object print depth #420

inadarei commented Jan 2, 2018

ljharb left a comment

ljharb Jan 2, 2018

inadarei Jan 2, 2018

ljharb Jan 3, 2018

ljharb Jan 3, 2018

inadarei Jan 3, 2018

ljharb Jan 3, 2018

inadarei Jan 3, 2018

ljharb Jan 4, 2018

ljharb Jan 4, 2018

ljharb Jan 4, 2018

inadarei Jan 4, 2018

ljharb Jan 4, 2018

inadarei Jan 4, 2018

ljharb Jan 5, 2018

ljharb Jan 5, 2018

ljharb left a comment

inadarei commented Jan 10, 2018

		@@ -15,10 +15,10 @@ var test = require('tape');

		test('timing test', function (t) {
		t.plan(2);

[New] use process.env.NODE_TAPE_OBJECT_PRINT_DEPTH for the default object print depth #420

[New] use process.env.NODE_TAPE_OBJECT_PRINT_DEPTH for the default object print depth #420

Conversation

inadarei commented Jan 2, 2018

ljharb 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

ljharb left a comment

Choose a reason for hiding this comment

inadarei commented Jan 10, 2018

[New] use `process.env.NODE_TAPE_OBJECT_PRINT_DEPTH` for the default object print depth #420

[New] use `process.env.NODE_TAPE_OBJECT_PRINT_DEPTH` for the default object print depth #420