Organize the source files in the OAuth package #1430

seratch · 2022-02-21T08:46:13Z

Summary

As a preparation for my forthcoming pull requests, this patch splits the big index.ts file into smaller pieces of code. This change improves the maintainability of both the main code and test code.

The changes in this PR are:

Moved many parts from index.ts
- Moved ClearStateState from the file to ./src/state-stores/clear-state-store.ts
- Moved other public interface from the file to ./src/state-stores/interface.ts
- Moved InstallationStore from the file to ./src/stores/interface.ts
- Also, moved many tests from ./src/index.spec.js to relevant spec files
- Moved InstallProvider, CallbackOptions, InstallURLOptions, InstallProviderOptions, Installation, InstallationQuery to dedicated files
- Added tests for all of the above
- The exports in src/index.ts are fully backward-compatible
Added a new test for logger.ts

I will rebase #1425 after merging this PR.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

seratch

comments for reviewers

seratch · 2022-02-21T08:52:49Z

packages/oauth/package.json

@@ -33,7 +33,7 @@
    "build:clean": "shx rm -rf ./dist ./coverage ./.nyc_output",
    "lint": "eslint --ext .ts src",
    "test": "npm run lint && npm run test:mocha",
-    "test:mocha": "nyc mocha --config .mocharc.json src/*.spec.js",
+    "test:mocha": "nyc mocha --config .mocharc.json src/*.spec.js src/**/*.spec.js src/*.spec.ts src/**/*.spec.ts",


As I've added more tests including the ones in TypeScript, this change is necessary for running all

seratch · 2022-02-21T08:53:03Z

packages/oauth/package.json

@@ -61,7 +63,7 @@
    "eslint-plugin-node": "^11.1.0",
    "mocha": "^9.1.0",
    "nop": "^1.0.0",
-    "nyc": "^14.1.1",
+    "nyc": "^15.1.0",


for fixing npm audit warnings

seratch · 2022-02-21T08:53:27Z

packages/oauth/src/callback-options.spec.ts

+import { MissingStateError } from './errors';
+
+describe('CallbackOptions', async () => {
+  it('should have success and failure', async () => {


These tests are not so great but it's better than nothing

Indeed, only about ~62% of branches are tested in this package, so definitely room for improvement!

seratch · 2022-02-21T08:54:24Z

packages/oauth/src/install-provider.spec.js

@@ -631,232 +630,4 @@ describe('OAuth', async () => {
      assert.equal(fakeStateStore.verifyStateParam.callCount, 0);
    });
  });
-
-  describe('MemoryInstallationStore', async () => {


These deleted tests are moved to more relevant files.

seratch · 2022-02-21T08:55:57Z

packages/oauth/src/install-provider.ts

+// This is intentionally structurally identical to AuthorizeResult from App
+// It is redefined so that this class remains loosely coupled to the rest
+// of Bolt.
+export interface AuthorizeResult {


We may want to have dedicated files for these interface. However, I didn't do so at this moment because these are used only in InstallProvider. If we want to make it more granular, we can do so in future changes.

Also, these are exported in index.ts as with before

Revisited this and I came to think that AuthorizeResult should be authorize-result.ts as external code relies on this interface.

seratch · 2022-02-21T08:57:03Z

packages/oauth/src/logger.spec.js

+    assert.match(newLogger.name, /test-logger-name:\d+/);
+    assert.equal(newLogger.getLevel(), LogLevel.DEBUG);
+  });
+  it('should modify the passed logger', async () => {


To make this clear, I've added this test

seratch · 2022-02-21T08:58:08Z

packages/oauth/src/stores/file-store.spec.js

+  isEnterpriseInstall: false,
+}
+
+// TODO: valid tests with org-wide installations


We should add more tests for org-wide installations and token rotation use cases

seratch · 2022-02-21T08:59:24Z

packages/oauth/tsconfig.json

@@ -30,6 +30,7 @@
  ],
  "exclude": [
    "src/**/*.spec.js",
+    "src/**/*.spec.ts",


Added a few TS tests. They should be excluded in the package file. I've verified if this works by running npm pack.

seratch · 2022-02-21T09:00:53Z

packages/oauth/src/stores/file-store.spec.js

+  });
+
+  it('should store the latest installation', async () => {
+    const installationStore = new FileInstallationStore({ baseDir: './tmp' });


Changed the base directory for outputs - previously the tests used to generate directories in current dir.

Not necessary as a change but another option is to use os.tmpDir()

Oh, nice! will update this part.

filmaj

LGTM! Ran it locally. Also much easier now to figure out where the missing code coverage is (via npm it && nyc report --reporter=text) - thanks a lot for doing this.

filmaj · 2022-02-22T18:00:41Z

packages/oauth/src/callback-options.spec.ts

+import { MissingStateError } from './errors';
+
+describe('CallbackOptions', async () => {
+  it('should have success and failure', async () => {


Indeed, only about ~62% of branches are tested in this package, so definitely room for improvement!

filmaj · 2022-02-22T18:17:10Z

packages/oauth/src/stores/file-store.spec.js

+  });
+
+  it('should store the latest installation', async () => {
+    const installationStore = new FileInstallationStore({ baseDir: './tmp' });


Not necessary as a change but another option is to use os.tmpDir()

seratch · 2022-02-22T22:53:47Z

Thanks for your review @filmaj !

seratch added enhancement M-T: A feature request for new functionality pkg:oauth applies to `@slack/oauth-helper` labels Feb 21, 2022

seratch added this to the oauth@2.5.0 milestone Feb 21, 2022

seratch requested review from filmaj and stevengill February 21, 2022 08:46

seratch self-assigned this Feb 21, 2022

seratch commented Feb 21, 2022

View reviewed changes

filmaj approved these changes Feb 22, 2022

View reviewed changes

seratch added 4 commits February 23, 2022 07:44

Organize the source files in the OAuth package

e15c447

Add clear TODO comments in tests

10c9bc4

Extract AuthorizeResult from install-provider.ts

be5dfaf

Update tests

48d1c61

seratch force-pushed the organize-oauth-package branch from 512f2b1 to 48d1c61 Compare February 22, 2022 22:49

seratch merged commit 6f8fa31 into slackapi:main Feb 22, 2022

seratch deleted the organize-oauth-package branch February 22, 2022 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Organize the source files in the OAuth package #1430

Organize the source files in the OAuth package #1430

seratch commented Feb 21, 2022

seratch left a comment

seratch Feb 21, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

filmaj Feb 22, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

seratch Feb 21, 2022

filmaj Feb 22, 2022

seratch Feb 22, 2022

filmaj left a comment

filmaj Feb 22, 2022

filmaj Feb 22, 2022

seratch commented Feb 22, 2022

Organize the source files in the OAuth package #1430

Organize the source files in the OAuth package #1430

Conversation

seratch commented Feb 21, 2022

Summary

Requirements (place an x in each [ ])

seratch 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

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seratch commented Feb 22, 2022

Requirements (place an `x` in each `[ ]`)