Skip to content

Commit ae3f1bf

Browse files
cursoragentcursoragent
committed
Fix context abstraction bugs: remove unused properties and fix env access
- Remove unused properties (stderr, ci, noColor, forceColor) from describeContext - Add env property to context object to capture process.env - Update buildEnvironment to read from context.env instead of process.env directly - Update all tests to pass context with env property for proper isolation
1 parent 775ac15 commit ae3f1bf

5 files changed

Lines changed: 22 additions & 91 deletions

File tree

‎test/build-exec-environment.spec.js‎

Lines changed: 15 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -32,78 +32,71 @@ describe('build-exec-environment', () => {
3232

3333
describe('inherited vars', () => {
3434
it('should forward TERM when set', () => {
35-
process.env.TERM = 'xterm-256color';
36-
const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
35+
const ctx = {stdout: {isTTY: true}, env: {TERM: 'xterm-256color'}};
3736
const env = buildEnvironment(ctx);
3837
expect(env.TERM).to.equal('xterm-256color');
3938
});
4039

4140
it('should not include TERM when unset', () => {
42-
const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
41+
const ctx = {stdout: {isTTY: true}, env: {}};
4342
const env = buildEnvironment(ctx);
4443
expect(env).to.not.have.property('TERM');
4544
});
4645

4746
it('should forward CI when set', () => {
48-
process.env.CI = 'true';
49-
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
47+
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {CI: 'true'}};
5048
const env = buildEnvironment(ctx);
5149
expect(env.CI).to.equal('true');
5250
});
5351

5452
it('should forward DEBUG when set', () => {
55-
process.env.DEBUG = '*';
56-
const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
53+
const ctx = {stdout: {isTTY: true}, env: {DEBUG: '*'}};
5754
const env = buildEnvironment(ctx);
5855
expect(env.DEBUG).to.equal('*');
5956
});
6057

6158
it('should forward TZ when set', () => {
62-
process.env.TZ = 'America/New_York';
63-
const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
59+
const ctx = {stdout: {isTTY: true}, env: {TZ: 'America/New_York'}};
6460
const env = buildEnvironment(ctx);
6561
expect(env.TZ).to.equal('America/New_York');
6662
});
6763

6864
it('should not forward FORCE_COLOR when stdout is not a TTY', () => {
69-
process.env.FORCE_COLOR = '1';
70-
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
65+
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {FORCE_COLOR: '1'}};
7166
const env = buildEnvironment(ctx);
7267
expect(env).to.not.have.property('FORCE_COLOR');
7368
});
7469

7570
it('should not forward CLICOLOR_FORCE when stdout is not a TTY', () => {
76-
process.env.CLICOLOR_FORCE = '3';
77-
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
71+
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {CLICOLOR_FORCE: '3'}};
7872
const env = buildEnvironment(ctx);
7973
expect(env).to.not.have.property('CLICOLOR_FORCE');
8074
});
8175

8276
it('should still forward NO_COLOR when stdout is not a TTY', () => {
83-
process.env.NO_COLOR = '1';
84-
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: true};
77+
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {NO_COLOR: '1'}};
8578
const env = buildEnvironment(ctx);
8679
expect(env.NO_COLOR).to.equal('1');
8780
});
8881
});
8982

9083
describe('synthetic vars', () => {
9184
it('should set COLUMNS and LINES when stdout is not a TTY', () => {
92-
const ctx = {stdout: {isTTY: false, columns: 120, rows: 40}, stderr: {isTTY: false}, noColor: false};
85+
const ctx = {stdout: {isTTY: false, columns: 120, rows: 40}, env: {}};
9386
const env = buildEnvironment(ctx);
9487
expect(env.COLUMNS).to.equal('120');
9588
expect(env.LINES).to.equal('40');
9689
});
9790

9891
it('should not set COLUMNS and LINES when stdout is a TTY', () => {
99-
const ctx = {stdout: {isTTY: true, columns: 120, rows: 40}, stderr: {isTTY: true}, noColor: false};
92+
const ctx = {stdout: {isTTY: true, columns: 120, rows: 40}, env: {}};
10093
const env = buildEnvironment(ctx);
10194
expect(env).to.not.have.property('COLUMNS');
10295
expect(env).to.not.have.property('LINES');
10396
});
10497

10598
it('should not synthetically set CLICOLOR_FORCE when stdout is piped', () => {
106-
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: true}, noColor: false};
99+
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {}};
107100
const env = buildEnvironment(ctx);
108101
// CLICOLOR_FORCE affects all streams, not just stderr.
109102
expect(env).to.not.have.property('CLICOLOR_FORCE');
@@ -112,29 +105,27 @@ describe('build-exec-environment', () => {
112105

113106
describe('user overrides', () => {
114107
it('should let user env override inherited vars', () => {
115-
process.env.TERM = 'xterm';
116-
const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
108+
const ctx = {stdout: {isTTY: true}, env: {TERM: 'xterm'}};
117109
const env = buildEnvironment(ctx, {TERM: 'dumb'});
118110
expect(env.TERM).to.equal('dumb');
119111
});
120112

121113
it('should let user env override synthetic vars', () => {
122-
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
114+
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {}};
123115
const env = buildEnvironment(ctx, {COLUMNS: '200'});
124116
expect(env.COLUMNS).to.equal('200');
125117
});
126118

127119
it('should pass through arbitrary user vars', () => {
128-
const ctx = {stdout: {isTTY: true}, stderr: {isTTY: true}, noColor: false};
120+
const ctx = {stdout: {isTTY: true}, env: {}};
129121
const env = buildEnvironment(ctx, {MY_APP_VAR: 'hello'});
130122
expect(env.MY_APP_VAR).to.equal('hello');
131123
});
132124
});
133125

134126
describe('precedence', () => {
135127
it('should apply inherited < synthetic < user', () => {
136-
process.env.FORCE_COLOR = '1';
137-
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, stderr: {isTTY: false}, noColor: false};
128+
const ctx = {stdout: {isTTY: false, columns: 80, rows: 24}, env: {FORCE_COLOR: '1'}};
138129
const env = buildEnvironment(ctx, {COLUMNS: '999', FORCE_COLOR: '3'});
139130
// User wins over synthetic
140131
expect(env.COLUMNS).to.equal('999');

‎test/describe-context.spec.js‎

Lines changed: 2 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -14,33 +14,20 @@ const describeContext = require('../utils/describe-context');
1414
describe('describe-context', () => {
1515
const originalStdinIsTTY = process.stdin.isTTY;
1616
const originalStdoutIsTTY = process.stdout.isTTY;
17-
const originalStderrIsTTY = process.stderr.isTTY;
1817
const originalLando = process.lando;
19-
const originalEnv = {...process.env};
2018

2119
afterEach(() => {
2220
process.stdin.isTTY = originalStdinIsTTY;
2321
process.stdout.isTTY = originalStdoutIsTTY;
24-
process.stderr.isTTY = originalStderrIsTTY;
2522
process.lando = originalLando;
26-
// Restore env vars we may have changed
27-
delete process.env.CI;
28-
delete process.env.NO_COLOR;
29-
delete process.env.FORCE_COLOR;
30-
if (originalEnv.CI !== undefined) process.env.CI = originalEnv.CI;
31-
if (originalEnv.NO_COLOR !== undefined) process.env.NO_COLOR = originalEnv.NO_COLOR;
32-
if (originalEnv.FORCE_COLOR !== undefined) process.env.FORCE_COLOR = originalEnv.FORCE_COLOR;
3323
});
3424

35-
it('should return an object with stdin, stdout, stderr, and flags', () => {
25+
it('should return an object with stdin, stdout, isNodeMode, and env', () => {
3626
const ctx = describeContext();
3727
expect(ctx).to.have.property('stdin');
3828
expect(ctx).to.have.property('stdout');
39-
expect(ctx).to.have.property('stderr');
4029
expect(ctx).to.have.property('isNodeMode');
41-
expect(ctx).to.have.property('ci');
42-
expect(ctx).to.have.property('noColor');
43-
expect(ctx).to.have.property('forceColor');
30+
expect(ctx).to.have.property('env');
4431
});
4532

4633
it('should reflect stdin TTY state', () => {
@@ -59,13 +46,6 @@ describe('describe-context', () => {
5946
expect(describeContext().stdout.isTTY).to.be.false;
6047
});
6148

62-
it('should reflect stderr TTY state', () => {
63-
process.stderr.isTTY = true;
64-
expect(describeContext().stderr.isTTY).to.be.true;
65-
66-
process.stderr.isTTY = undefined;
67-
expect(describeContext().stderr.isTTY).to.be.false;
68-
});
6949

7050
it('should default stdout columns and rows when not available', () => {
7151
const ctx = describeContext();
@@ -86,36 +66,4 @@ describe('describe-context', () => {
8666
delete process.lando;
8767
expect(describeContext().isNodeMode).to.be.false;
8868
});
89-
90-
it('should detect CI from environment', () => {
91-
process.env.CI = 'true';
92-
expect(describeContext().ci).to.be.true;
93-
94-
delete process.env.CI;
95-
expect(describeContext().ci).to.be.false;
96-
});
97-
98-
it('should detect NO_COLOR from environment', () => {
99-
process.env.NO_COLOR = '1';
100-
expect(describeContext().noColor).to.be.true;
101-
102-
delete process.env.NO_COLOR;
103-
expect(describeContext().noColor).to.be.false;
104-
});
105-
106-
it('should detect NO_COLOR when set to an empty string', () => {
107-
process.env.NO_COLOR = '';
108-
expect(describeContext().noColor).to.be.true;
109-
110-
delete process.env.NO_COLOR;
111-
expect(describeContext().noColor).to.be.false;
112-
});
113-
114-
it('should capture FORCE_COLOR from environment', () => {
115-
process.env.FORCE_COLOR = '3';
116-
expect(describeContext().forceColor).to.equal('3');
117-
118-
delete process.env.FORCE_COLOR;
119-
expect(describeContext().forceColor).to.be.undefined;
120-
});
12169
});

‎test/tty-allocation.spec.js‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,13 @@ const {buildExecArgs} = require('../utils/build-docker-exec');
2121

2222
// Helper to build a minimal context object for testing
2323
const makeContext = (overrides = {}) => {
24-
const {stdin, stdout, stderr, ...rest} = overrides;
24+
const {stdin, stdout, env, ...rest} = overrides;
2525

2626
return {
2727
stdin: {isTTY: false, isClosed: false, ...stdin},
2828
stdout: {isTTY: false, columns: 80, rows: 24, ...stdout},
29-
stderr: {isTTY: false, ...stderr},
3029
isNodeMode: true,
31-
ci: false,
32-
noColor: false,
33-
forceColor: undefined,
30+
env: env || {},
3431
...rest,
3532
};
3633
};

‎utils/build-exec-environment.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,13 +25,13 @@ const forceColorKeys = ['FORCE_COLOR', 'CLICOLOR_FORCE'];
2525
module.exports = (context, userEnv = {}) => {
2626
const inherited = {};
2727
for (const key of forwardKeys) {
28-
if (process.env[key] === undefined) continue;
28+
if (context.env[key] === undefined) continue;
2929

3030
// Redirected stdout should not inherit env vars that force color,
3131
// or they can bypass the no-TTY safeguard and reintroduce ANSI codes.
3232
if (!context.stdout.isTTY && forceColorKeys.includes(key)) continue;
3333

34-
inherited[key] = process.env[key];
34+
inherited[key] = context.env[key];
3535
}
3636

3737
const synthetic = {};

‎utils/describe-context.js‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,6 @@ module.exports = () => ({
1717
columns: process.stdout.columns || 80,
1818
rows: process.stdout.rows || 24,
1919
},
20-
stderr: {
21-
isTTY: Boolean(process.stderr.isTTY),
22-
},
2320
isNodeMode: process.lando === 'node',
24-
ci: Boolean(process.env.CI),
25-
noColor: process.env.NO_COLOR !== undefined,
26-
forceColor: process.env.FORCE_COLOR,
21+
env: process.env,
2722
});

0 commit comments

Comments
 (0)