Skip to content

Commit c262ed9

Browse files
ecraig12345ecraig12345
committed
terrible fix
1 parent c7d930b commit c262ed9

12 files changed

Lines changed: 75 additions & 52 deletions

File tree

‎.github/workflows/pr.yml‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,17 @@ jobs:
7474
if: matrix.os == 'ubuntu-latest' && matrix.node == 14 && matrix.npm == 8
7575

7676
- run: yarn lint:ci
77+
if: matrix.os == 'ubuntu-latest' && matrix.node == 14 && matrix.npm == 8
7778

7879
- run: yarn test --verbose
7980
id: test
8081

82+
# See packageManager.ts comments...
83+
- name: test publish (Windows bash)
84+
if: matrix.os == 'windows-latest'
85+
shell: bash
86+
run: yarn workspace beachball test packagePublish
87+
8188
# For a few specific test failures, it can be helpful to see npm debug logs
8289
# (usually not relevant, but there's not a good way to distinguish by specific failure)
8390
- name: (on test failure) Get npm cache path

‎packages/beachball/src/__e2e__/syncE2E.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@ describe('sync command (e2e)', () => {
3131
const publishOptions: Parameters<typeof packagePublish>[1] = {
3232
registry: 'fake',
3333
retries: 3,
34-
path: undefined,
34+
path: '',
3535
npmReadConcurrency: 2,
3636
};
3737

‎packages/beachball/src/__fixtures__/mockNpm.test.ts‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -134,50 +134,48 @@ describe('_mockNpmShow', () => {
134134

135135
it("errors if package doesn't exist", async () => {
136136
const emptyData = _makeRegistryData({});
137-
const result = await _mockNpmShow(emptyData, ['foo'], { cwd: undefined });
137+
const result = await _mockNpmShow(emptyData, ['foo'], { cwd: '' });
138138
expect(result).toEqual(getErrorResult('[fake] code E404 - foo - not found'));
139139
});
140140

141141
it('returns requested version plus dist-tags and version list', async () => {
142-
const result = await _mockNpmShow(data, ['foo@1.0.0'], { cwd: undefined });
142+
const result = await _mockNpmShow(data, ['foo@1.0.0'], { cwd: '' });
143143
expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.0' }));
144144
});
145145

146146
it('returns requested version of scoped package', async () => {
147-
const result = await _mockNpmShow(data, ['@foo/bar@2.0.0'], { cwd: undefined });
147+
const result = await _mockNpmShow(data, ['@foo/bar@2.0.0'], { cwd: '' });
148148
expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.0' }));
149149
});
150150

151151
it('returns requested tag', async () => {
152-
const result = await _mockNpmShow(data, ['foo@beta'], { cwd: undefined });
152+
const result = await _mockNpmShow(data, ['foo@beta'], { cwd: '' });
153153
expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.0-beta' }));
154154
});
155155

156156
it('returns requested tag of scoped package', async () => {
157-
const result = await _mockNpmShow(data, ['@foo/bar@beta'], { cwd: undefined });
157+
const result = await _mockNpmShow(data, ['@foo/bar@beta'], { cwd: '' });
158158
expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.0-beta' }));
159159
});
160160

161161
it('returns latest version if no version requested', async () => {
162-
const result = await _mockNpmShow(data, ['foo'], { cwd: undefined });
162+
const result = await _mockNpmShow(data, ['foo'], { cwd: '' });
163163
expect(result).toEqual(getShowResult({ name: 'foo', version: '1.0.1' }));
164164
});
165165

166166
it('returns latest version of scoped package if no version requested', async () => {
167-
const result = await _mockNpmShow(data, ['@foo/bar'], { cwd: undefined });
167+
const result = await _mockNpmShow(data, ['@foo/bar'], { cwd: '' });
168168
expect(result).toEqual(getShowResult({ name: '@foo/bar', version: '2.0.1' }));
169169
});
170170

171171
it("errors if requested version doesn't exist", async () => {
172-
const result = await _mockNpmShow(data, ['foo@2.0.0'], { cwd: undefined });
172+
const result = await _mockNpmShow(data, ['foo@2.0.0'], { cwd: '' });
173173
expect(result).toEqual(getErrorResult('[fake] code E404 - foo@2.0.0 - not found'));
174174
});
175175

176176
// support for this could be added later
177177
it('currently throws if requested version is a range', async () => {
178-
await expect(() => _mockNpmShow(data, ['foo@^1.0.0'], { cwd: undefined })).rejects.toThrow(
179-
/not currently supported/
180-
);
178+
await expect(() => _mockNpmShow(data, ['foo@^1.0.0'], { cwd: '' })).rejects.toThrow(/not currently supported/);
181179
});
182180
});
183181

@@ -209,9 +207,7 @@ describe('_mockNpmPublish', () => {
209207
});
210208

211209
it('throws if cwd is not specified', async () => {
212-
await expect(() => _mockNpmPublish({}, [], { cwd: undefined })).rejects.toThrow(
213-
'cwd is required for mock npm publish'
214-
);
210+
await expect(() => _mockNpmPublish({}, [], { cwd: '' })).rejects.toThrow('cwd is required for mock npm publish');
215211
});
216212

217213
it('errors if reading package.json fails', async () => {
@@ -308,7 +304,7 @@ describe('_mockNpmPack', () => {
308304
});
309305

310306
it('throws if cwd is not specified', async () => {
311-
await expect(() => _mockNpmPack({}, [], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm pack');
307+
await expect(() => _mockNpmPack({}, [], { cwd: '' })).rejects.toThrow('cwd is required for mock npm pack');
312308
});
313309

314310
it('errors if reading package.json fails', async () => {
@@ -433,7 +429,7 @@ describe('mockNpm', () => {
433429
versions: ['1.0.0'],
434430
'dist-tags': { latest: '1.0.0' },
435431
});
436-
expect(await npm(['show', 'foo'], { cwd: undefined })).toMatchObject({
432+
expect(await npm(['show', 'foo'], { cwd: '' })).toMatchObject({
437433
success: true,
438434
stdout: JSON.stringify({
439435
name: 'foo',
@@ -485,19 +481,19 @@ describe('mockNpm', () => {
485481
});
486482

487483
it('throws if required cwd is missing', async () => {
488-
await expect(() => npm(['publish'], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm publish');
489-
await expect(() => npm(['pack'], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm pack');
484+
await expect(() => npm(['publish'], { cwd: '' })).rejects.toThrow('cwd is required for mock npm publish');
485+
await expect(() => npm(['pack'], { cwd: '' })).rejects.toThrow('cwd is required for mock npm pack');
490486
});
491487

492488
it('throws on unsupported command', async () => {
493-
await expect(() => npm(['foo'], { cwd: undefined })).rejects.toThrow('Command not supported by mock npm: foo');
489+
await expect(() => npm(['foo'], { cwd: '' })).rejects.toThrow('Command not supported by mock npm: foo');
494490
expect(npmMock.mock).toHaveBeenCalledTimes(1);
495-
expect(npmMock.mock).toHaveBeenCalledWith(['foo'], expect.objectContaining({ cwd: undefined }));
491+
expect(npmMock.mock).toHaveBeenCalledWith(['foo'], expect.objectContaining({ cwd: '' }));
496492
});
497493

498494
it('TEMP mocks npm show command', async () => {
499495
npmMock.setRegistryData({ foo: { versions: ['1.0.0'] } });
500-
const result = await npm(['show', 'foo'], { cwd: undefined });
496+
const result = await npm(['show', 'foo'], { cwd: '' });
501497
expect(result).toMatchObject({
502498
success: true,
503499
stdout: expect.stringContaining('"name":"foo"'),
@@ -511,24 +507,24 @@ describe('mockNpm', () => {
511507
it('respects mocked command override', async () => {
512508
const mockPublish = jest.fn(() => Promise.resolve(fakePublishResult as unknown as MockNpmResult));
513509
npmMock.setCommandOverride('publish', mockPublish);
514-
const result = await npm(['publish', 'foo'], { cwd: undefined });
510+
const result = await npm(['publish', 'foo'], { cwd: '' });
515511
expect(result).toEqual(fakePublishResult);
516-
expect(mockPublish).toHaveBeenCalledWith(expect.any(Object), ['foo'], { cwd: undefined });
512+
expect(mockPublish).toHaveBeenCalledWith(expect.any(Object), ['foo'], { cwd: '' });
517513
});
518514

519515
it("respects extra mocked command that's not normally supported", async () => {
520516
const mockFoo = jest.fn(() => Promise.resolve('hi' as unknown as MockNpmResult));
521517
npmMock.setCommandOverride('foo', mockFoo);
522-
const result = await npm(['foo'], { cwd: undefined });
518+
const result = await npm(['foo'], { cwd: '' });
523519
expect(result).toEqual('hi');
524-
expect(mockFoo).toHaveBeenCalledWith(expect.any(Object), [], { cwd: undefined });
520+
expect(mockFoo).toHaveBeenCalledWith(expect.any(Object), [], { cwd: '' });
525521
});
526522

527523
it('resets commands after each test', async () => {
528524
// extra command is gone
529-
await expect(() => npm(['foo'], { cwd: undefined })).rejects.toThrow('Command not supported by mock npm: foo');
525+
await expect(() => npm(['foo'], { cwd: '' })).rejects.toThrow('Command not supported by mock npm: foo');
530526
// publish mock is gone
531-
await expect(() => npm(['publish'], { cwd: undefined })).rejects.toThrow('cwd is required for mock npm publish');
527+
await expect(() => npm(['publish'], { cwd: '' })).rejects.toThrow('cwd is required for mock npm publish');
532528
});
533529
});
534530
});

‎packages/beachball/src/__functional__/packageManager/getNpmPackageInfo.test.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,13 @@ describe('getNpmPackageInfo', () => {
2929
{ desc: 'scoped', name: '@lage-run/cli', knownVersion: '0.33.0' },
3030
])('gets info for $desc package from public npm registry', async ({ name, knownVersion }) => {
3131
const timeout = 10000;
32-
const result = await getNpmPackageInfo(name, { registry, timeout, path: undefined });
32+
const result = await getNpmPackageInfo(name, { registry, timeout, path: '' });
3333

3434
expect(npmSpy).toHaveBeenCalledTimes(1);
3535
// Verify args format
3636
expect(npmSpy).toHaveBeenCalledWith(
3737
['show', '--registry', registry, '--json', name, ..._npmShowProperties],
38-
expect.objectContaining({ timeout, cwd: undefined })
38+
expect.objectContaining({ timeout, cwd: '' })
3939
);
4040
// Verify output format (there's no toHaveResolvedWith matcher, so await the return value)
4141
expect(await npmSpy.mock.results[0].value).toMatchObject({
@@ -91,7 +91,7 @@ describe('getNpmPackageInfo', () => {
9191

9292
it('passes auth args', async () => {
9393
// Don't care about the result in this case
94-
await getNpmPackageInfo(shouldNotExist, { registry, token: 'fake', path: undefined });
94+
await getNpmPackageInfo(shouldNotExist, { registry, token: 'fake', path: '' });
9595

9696
expect(npmSpy).toHaveBeenCalledTimes(1);
9797
expect(npmSpy).toHaveBeenCalledWith(

‎packages/beachball/src/__functional__/packageManager/packagePublish.test.ts‎

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,10 @@ describe('packagePublish', () => {
3838

3939
const logs = initMockLogs();
4040

41-
const defaultOptions: Omit<PackagePublishOptions, 'path' | 'registry'> = {
41+
const defaultOptions: Omit<PackagePublishOptions, 'path'> = {
4242
npmReadConcurrency: 2,
4343
retries: 3,
44+
registry: 'http://fake-registry', // overwritten for real tests
4445
};
4546

4647
function getTestPackageInfo(): PackageInfo {
@@ -184,10 +185,11 @@ describe('packagePublish', () => {
184185
expect(logs2ndTry).toMatch(`${testSpec} already exists in the registry`);
185186
});
186187

187-
// TODO: remove condition once node version is upgraded (this test doesn't work with npm 6 because
188-
// that version seems to allow truly anonymous publishing with verdaccio)
189188
it('handles auth error and does not retry', async () => {
189+
// TODO: remove condition once node version is upgraded (this test doesn't work with npm 6 because
190+
// that version seems to allow truly anonymous publishing with verdaccio)
190191
if (npmVersion.startsWith('6.')) {
192+
console.warn('Skipping auth error test on npm 6');
191193
return;
192194
}
193195
await registry.logout();
@@ -212,7 +214,6 @@ describe('packagePublish', () => {
212214

213215
const publishResult = await packagePublish(testPackageInfo, {
214216
...defaultOptions,
215-
registry: 'http://fake-registry',
216217
path: tempRoot,
217218
token: 'fake-token',
218219
});
@@ -244,7 +245,6 @@ describe('packagePublish', () => {
244245

245246
const publishResult = await packagePublish(testPackageInfo, {
246247
...defaultOptions,
247-
registry: 'fake',
248248
path: tempRoot,
249249
});
250250
expect(publishResult).toEqual(successResult);
@@ -264,7 +264,6 @@ describe('packagePublish', () => {
264264

265265
const publishResult = await packagePublish(getTestPackageInfo(), {
266266
...defaultOptions,
267-
registry: 'fake',
268267
path: tempRoot,
269268
});
270269
expect(publishResult).toEqual(failedResult);
@@ -284,7 +283,6 @@ describe('packagePublish', () => {
284283

285284
const publishResult = await packagePublish(testPackageInfo, {
286285
...defaultOptions,
287-
registry: 'fake',
288286
path: tempRoot,
289287
});
290288
expect(publishResult).toEqual(failedResult);
@@ -303,7 +301,6 @@ describe('packagePublish', () => {
303301

304302
const publishResult = await packagePublish(testPackageInfo, {
305303
...defaultOptions,
306-
registry: 'fake',
307304
path: tempRoot,
308305
});
309306
expect(publishResult).toEqual(failedResult);
@@ -318,7 +315,6 @@ describe('packagePublish', () => {
318315

319316
const publishResult = await packagePublish(testPackageInfo, {
320317
...defaultOptions,
321-
registry: 'fake',
322318
path: tempRoot,
323319
});
324320
expect(publishResult).toEqual(failedResult);

‎packages/beachball/src/__functional__/publish/publishToRegistry.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ describe('publishToRegistry', () => {
117117

118118
// lib should be published before app (check npm call order)
119119
const publishCalls = npmMock.mock.mock.calls.filter(([args]) => args[0] === 'publish');
120-
const publishOrder = publishCalls.map(([, opts]) => path.basename(opts.cwd!));
120+
const publishOrder = publishCalls.map(([, opts]) => path.basename(opts.cwd));
121121
expect(publishOrder).toEqual(['lib', 'app']);
122122

123123
expect(logs.getMockLines('all', { root: tempRoot })).toMatchSnapshot();
@@ -340,7 +340,7 @@ describe('publishToRegistry', () => {
340340
// Make publish fail for pkg2 and pkg5.
341341
// pkg3 and pkg4 should succeed, and pkg1 should be skipped.
342342
npmMock.setCommandOverride('publish', async (registryData, args, opts) => {
343-
const packageName = path.basename(opts.cwd!);
343+
const packageName = path.basename(opts.cwd);
344344
if (packageName === 'pkg2' || packageName === 'pkg5') {
345345
return { success: false, stdout: '', stderr: 'oh no', all: 'oh no', failed: true };
346346
}

‎packages/beachball/src/__tests__/packageManager/listPackageVersions.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ describe('list npm versions', () => {
2626
const npmOptions: NpmOptions = {
2727
registry,
2828
timeout,
29-
path: undefined,
29+
path: '',
3030
npmReadConcurrency: 2,
3131
};
3232

‎packages/beachball/src/__tests__/publish/getNewPackages.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ describe('getNewPackages', () => {
1212
const logs = initMockLogs();
1313
/** Mock the `npm show` command for `npmAsync` calls. This also handles cleanup after each test. */
1414
const npmMock = initNpmMock();
15-
const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: undefined, registry: 'https://fake' };
15+
const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: '', registry: 'https://fake' };
1616

1717
it('returns empty if no packages exist', async () => {
1818
const newPackages = await getNewPackages({ modifiedPackages: new Set(), packageInfos: {} }, npmOptions);

‎packages/beachball/src/__tests__/publish/validatePackageVersions.test.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ describe('validatePackageVersions', () => {
1212
const logs = initMockLogs();
1313
/** Mock the `npm show` command. This also handles cleanup after each test. */
1414
const npmMock = initNpmMock();
15-
const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: undefined, registry: 'https://fake' };
15+
const npmOptions: NpmOptions = { npmReadConcurrency: 2, path: '', registry: 'https://fake' };
1616

1717
it('succeeds with nothing to validate', async () => {
1818
expect(await validatePackageVersions([], {}, npmOptions)).toBe(true);

‎packages/beachball/src/packageManager/packageManager.ts‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,8 @@
11
import execa from 'execa';
2+
import path from 'path';
23

34
export type PackageManagerResult = execa.ExecaReturnValue & { success: boolean };
4-
export type PackageManagerOptions = execa.Options & { cwd: string | undefined };
5+
export type PackageManagerOptions = execa.Options & { cwd: string };
56

67
/**
78
* Run a package manager command. Returns the error result instead of throwing on failure.
@@ -15,12 +16,34 @@ export async function packageManager(
1516
args: string[],
1617
options: PackageManagerOptions
1718
): Promise<PackageManagerResult> {
19+
let pathEnv = options.env?.PATH || process.env.PATH;
20+
if (manager === 'npm' && pathEnv) {
21+
// Workaround for an issue on certain platforms/shells(?) if the parent command was run VIA yarn:
22+
// The auth environment variable (e.g. `npm_config_//someRegistry/:_authToken`) was not being
23+
// passed through to the child process. This might be because:
24+
// - Special characters such as / and : aren't valid in env var names for certain shells/platforms
25+
// - On every `yarn run ...` command, yarn makes temp directories like /<temp>/yarn--1776822418161-0.7992675923334178
26+
// with aliases for `node` and `yarn`. On Linux (and Mac), the `node` alias looks something like:
27+
// #!/bin/sh
28+
// exec "/path/to/node" "$@"
29+
// (see https://github.com/yarnpkg/yarn/issues/6685 for context)
30+
// - Best guess: invalid environment variable names are dropped by this extra `exec` step??
31+
// (This consistently reproed on Ubuntu+bash, but not Mac+zsh or bash. The clue was that the
32+
// tests passed even on Linux when run via debugTests.js, but failed when run via yarn test.)
33+
//
34+
// Removing the yarn-- segment from the PATH seems to consistently fix this issue.
35+
pathEnv = pathEnv
36+
.split(path.delimiter)
37+
.filter(p => !path.basename(p).startsWith('yarn--'))
38+
.join(path.delimiter);
39+
}
40+
1841
try {
1942
const result = await execa(manager, args, {
2043
...options,
44+
env: { ...options.env, PATH: pathEnv },
2145
// This is required for Windows due to https://nodejs.org/en/blog/vulnerability/april-2024-security-releases-2
22-
// but only provide it on Windows because it breaks oddly-named environment variables
23-
// such as npm_config_//someRegistry/:_authToken on Linux...
46+
// but only provide it on Windows because it breaks the auth env var on Linux...
2447
...(process.platform === 'win32' && { shell: true }),
2548
});
2649
return {

0 commit comments

Comments
 (0)
⚡