From 0cdf6201269b24556c1b2fd711006c1879418342 Mon Sep 17 00:00:00 2001 From: Filip Maj Date: Tue, 29 Oct 2024 13:33:51 -0400 Subject: [PATCH 1/8] ci: add windows testing to CI by lifting OS into its own test matrix dimension --- .github/workflows/ci-build.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci-build.yml b/.github/workflows/ci-build.yml index e6eb6083a..ffb7587f0 100644 --- a/.github/workflows/ci-build.yml +++ b/.github/workflows/ci-build.yml @@ -8,11 +8,11 @@ on: jobs: test: - runs-on: ubuntu-latest timeout-minutes: 10 strategy: fail-fast: false matrix: + os: [ubuntu-latest, windows-latest] node-version: [18.x, 20.x, 22.x] package: - cli-hooks @@ -24,6 +24,7 @@ jobs: - types - web-api - webhook + runs-on: ${{ matrix.os }} steps: - uses: actions/checkout@v4 - name: Use Node.js ${{ matrix.node-version }} From fbb2d5025951b44c7fed6ec1f271d4cb19d05d8f Mon Sep 17 00:00:00 2001 From: Filip Maj Date: Tue, 29 Oct 2024 13:54:11 -0400 Subject: [PATCH 2/8] try some windows-specific approaches to npm linking --- .github/workflows/ci-build.yml | 39 +++++++++++++++++++++++++++++----- 1 file changed, 34 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci-build.yml b/.github/workflows/ci-build.yml index ffb7587f0..224bec94f 100644 --- a/.github/workflows/ci-build.yml +++ b/.github/workflows/ci-build.yml @@ -32,11 +32,13 @@ jobs: with: node-version: ${{ matrix.node-version }} - run: npm --version - - name: Build and Run Tests in Each Package + - run: npm install + working-directory: packages/${{ matrix.package }} + - name: Link dependent packages (*nix) + if: matrix.os == 'ubuntu-latest' working-directory: packages/${{ matrix.package }} run: | - npm install - # depending on which package we are testing, also npm link up other dependent packages + # depending on which package we are testing, also npm link up dependent packages within this monorepo case "$PWD" in */webhook) pushd ../types && npm i && popd && npm link ../types;; */web-api) pushd ../types && npm i && popd && npm link ../types && pushd ../logger && npm i && popd && npm link ../logger;; @@ -44,14 +46,41 @@ jobs: */socket-mode) pushd ../logger && npm i && popd && npm link ../logger && pushd ../web-api && npm i && popd && npm link ../web-api;; *) ;; # default esac - npm test + - name: Link dependent packages (Windows) + if: matrix.os == 'windows-latest' + working-directory: packages/${{ matrix.package }} + run: | + # depending on which package we are testing, also npm link up dependent packages within this monorepo + # NOTE: the following is PowerShell + echo "$pwd" + switch -Wildcard ( "$pwd" ) + { + '*\webhook' + { + pushd ..\types && npm i && popd && npm link ..\types + } + '*\web-api' + { + pushd ..\types && npm i && popd && npm link ..\types && pushd ..\logger && npm i && popd && npm link ..\logger + } + '*\oauth' + { + pushd ..\logger && npm i && popd && npm link ..\logger && pushd ..\web-api && npm i && popd && npm link ..\web-api + } + '*\socket-mode' + { + pushd ..\logger && npm i && popd && npm link ..\logger && pushd ..\web-api && npm i && popd && npm link ..\web-api + } + } + - run: npm test + working-directory: packages/${{ matrix.package }} - name: Check for coverage report existence id: check_coverage uses: andstor/file-existence-action@v3 with: files: packages/${{ matrix.package }}/coverage/lcov.info - name: Upload code coverage - if: matrix.node-version == '22.x' && steps.check_coverage.outputs.files_exists == 'true' + if: matrix.node-version == '22.x' && matrix.os == 'ubuntu-latest' && steps.check_coverage.outputs.files_exists == 'true' uses: codecov/codecov-action@v4 with: token: ${{ secrets.CODECOV_TOKEN }} From 79ef637b7a9d28e43843e63f9eff933c3e46c7a8 Mon Sep 17 00:00:00 2001 From: Filip Maj Date: Wed, 30 Oct 2024 09:49:51 -0400 Subject: [PATCH 3/8] debug user agent values on windows --- packages/web-api/src/WebClient.spec.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/web-api/src/WebClient.spec.ts b/packages/web-api/src/WebClient.spec.ts index a65c4e8d9..8895700f2 100644 --- a/packages/web-api/src/WebClient.spec.ts +++ b/packages/web-api/src/WebClient.spec.ts @@ -69,7 +69,7 @@ describe('WebClient', () => { it('should succeed when constructing a class that extends WebClient', () => { assert.doesNotThrow(() => { - class X extends WebClient {} + class X extends WebClient { } new X(); }); }); @@ -463,6 +463,7 @@ describe('WebClient', () => { const scope = nock('https://slack.com', { reqheaders: { 'User-Agent': (value) => { + console.log('User Agent is:', value); const metadata = parseUserAgentIntoMetadata(value); // NOTE: this assert isn't that strong and doesn't say anything about the values. at this time, there // isn't a good way to test this without dupicating the logic of the code under test. From 811478ab662845f5804f9715af5d1cb43cc49946 Mon Sep 17 00:00:00 2001 From: Filip Maj Date: Wed, 30 Oct 2024 10:40:53 -0400 Subject: [PATCH 4/8] what is process title on windows? --- packages/web-api/src/WebClient.spec.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/web-api/src/WebClient.spec.ts b/packages/web-api/src/WebClient.spec.ts index 8895700f2..7ec6c5a4d 100644 --- a/packages/web-api/src/WebClient.spec.ts +++ b/packages/web-api/src/WebClient.spec.ts @@ -463,7 +463,11 @@ describe('WebClient', () => { const scope = nock('https://slack.com', { reqheaders: { 'User-Agent': (value) => { + console.log('process title is:', process.title); console.log('User Agent is:', value); + // User Agent value is different across platforms. + // on mac this is: @slack:web-api/7.7.0 node/18.15.0 darwin/23.6.0 + // on windows this is: @slack:web-api/7.7.0 cmd.exe /22.10.0 win32/10.0.20348 const metadata = parseUserAgentIntoMetadata(value); // NOTE: this assert isn't that strong and doesn't say anything about the values. at this time, there // isn't a good way to test this without dupicating the logic of the code under test. From dfa6e9ccc4942dba1e320d40550536682d57ae77 Mon Sep 17 00:00:00 2001 From: Filip Maj Date: Wed, 30 Oct 2024 11:19:42 -0400 Subject: [PATCH 5/8] do not assume anything about contents of `process.title` when instrumenting user agent --- packages/web-api/src/WebClient.spec.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/packages/web-api/src/WebClient.spec.ts b/packages/web-api/src/WebClient.spec.ts index 7ec6c5a4d..c792dea99 100644 --- a/packages/web-api/src/WebClient.spec.ts +++ b/packages/web-api/src/WebClient.spec.ts @@ -463,19 +463,13 @@ describe('WebClient', () => { const scope = nock('https://slack.com', { reqheaders: { 'User-Agent': (value) => { - console.log('process title is:', process.title); - console.log('User Agent is:', value); // User Agent value is different across platforms. // on mac this is: @slack:web-api/7.7.0 node/18.15.0 darwin/23.6.0 // on windows this is: @slack:web-api/7.7.0 cmd.exe /22.10.0 win32/10.0.20348 const metadata = parseUserAgentIntoMetadata(value); // NOTE: this assert isn't that strong and doesn't say anything about the values. at this time, there // isn't a good way to test this without dupicating the logic of the code under test. - assert.containsAllKeys(metadata, ['node', '@slack:web-api']); - // NOTE: there's an assumption that if there's any keys besides these left at all, its the platform part - metadata.node = undefined; - metadata['@slack:client'] = undefined; - assert.isNotEmpty(metadata); + assert.containsAllKeys(metadata, ['@slack:web-api']); return true; }, }, From 019809959c8cd20a114f29fddde07dae030700b4 Mon Sep 17 00:00:00 2001 From: Filip Maj Date: Wed, 30 Oct 2024 11:36:27 -0400 Subject: [PATCH 6/8] tests: cli-test `runStop` `waitForShutdown` test should not run on Windows, since Windows does not support this option. --- packages/cli-test/src/cli/commands/platform.spec.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/packages/cli-test/src/cli/commands/platform.spec.ts b/packages/cli-test/src/cli/commands/platform.spec.ts index 108e55df5..d027b3a80 100644 --- a/packages/cli-test/src/cli/commands/platform.spec.ts +++ b/packages/cli-test/src/cli/commands/platform.spec.ts @@ -96,10 +96,12 @@ describe('platform commands', () => { sandbox.stub(shell, 'kill').rejects(); await assert.rejects(platform.runStop({ proc: fakeProcess })); }); - it('should reject if waitForShutdown=true and waitForOutput rejects', async () => { - sandbox.stub(shell, 'kill').resolves(); - waitForOutputSpy.rejects(); - await assert.rejects(platform.runStop({ proc: fakeProcess, waitForShutdown: true })); + it('non-Windows only: should reject if waitForShutdown=true and waitForOutput rejects', async () => { + if (process.platform !== 'win32') { + sandbox.stub(shell, 'kill').resolves(); + waitForOutputSpy.rejects(); + await assert.rejects(platform.runStop({ proc: fakeProcess, waitForShutdown: true })); + } }); it('should resolve immediately if waitForShutdown=false and shell.kill resolve', async () => { sandbox.stub(shell, 'kill').resolves(); From 6f3ca0a3f956f1cadc5e1f4000bd1bf6271533cf Mon Sep 17 00:00:00 2001 From: Filip Maj Date: Wed, 30 Oct 2024 11:41:16 -0400 Subject: [PATCH 7/8] tweak order of assertions in timeout test that fails more often on windows --- packages/web-api/src/WebClient.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/web-api/src/WebClient.spec.ts b/packages/web-api/src/WebClient.spec.ts index c792dea99..9cb0f5bf9 100644 --- a/packages/web-api/src/WebClient.spec.ts +++ b/packages/web-api/src/WebClient.spec.ts @@ -113,11 +113,11 @@ describe('WebClient', () => { } catch (e) { // biome-ignore lint/suspicious/noExplicitAny: TODO: type this better, should be whatever error class web-api throws for timeouts const error = e as any; - assert.isTrue((logger.warn as sinon.SinonStub).calledOnce, 'expected Logger to be called once'); assert.equal(error.code, ErrorCode.RequestError); assert.equal(error.original.config.timeout, timeoutOverride); assert.equal(error.original.isAxiosError, true); assert.instanceOf(error, Error); + assert.isTrue((logger.warn as sinon.SinonStub).calledOnce, 'expected Logger to be called once'); } }); }); From a92d53ba266f3869b9a07e40027c137bfb7f48c1 Mon Sep 17 00:00:00 2001 From: Fil Maj Date: Thu, 31 Oct 2024 12:47:52 -0400 Subject: [PATCH 8/8] Update packages/web-api/src/WebClient.spec.ts Co-authored-by: Michael Brooks --- packages/web-api/src/WebClient.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/web-api/src/WebClient.spec.ts b/packages/web-api/src/WebClient.spec.ts index 9cb0f5bf9..584016553 100644 --- a/packages/web-api/src/WebClient.spec.ts +++ b/packages/web-api/src/WebClient.spec.ts @@ -69,7 +69,7 @@ describe('WebClient', () => { it('should succeed when constructing a class that extends WebClient', () => { assert.doesNotThrow(() => { - class X extends WebClient { } + class X extends WebClient {} new X(); }); });