Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernize directory structure and refactor chat adapters #186

Merged
merged 9 commits into from
Jul 19, 2019

Conversation

blag
Copy link
Contributor

@blag blag commented Jul 17, 2019

Move scripts/stackstorm.js and lib/ into src/

I created the src/ directory and moved the code into it. This is done to separate the code from the repository "metadata" files, like package.json and test files.

Refactor chat provider adapters into their own modules

I broke up post-data.js and format-data.js into modules for their constituent chat providers. All of the provider-specific code is gathered up into one module, and all other modules are agnostic to the chat provider.

Thanks to inheritance, this greatly simplifies all adapters, because now they all inherit from the DefaultMessagingHandler and only override the functions they need to. This is especially true for the Slack-like adapters (Mattermost, Cisco Spark, and Rocketchat), so much so that consolidating and simplifying them down further can be done in the future (once we have a test system for them).

Breaking the adapters up should now also make it more obvious to casual users how to fix things for their provider adpater, and also how to create a new provider adapter from scratch. Hopefully this translates into more "drive by fixes".

Finally, because all of the adapters are broken up and simplified, we can (if we so choose) rewrite individual ones in coffeescript, which might be a bit more readable than Javascript and is reverse-compatible with Javascript. Rewriting them to use ES6 (along with class and "proper" inheritance with the extends keywords) is also a possibility.

Rename tests/ to test/

It's a small change, but I moved the tests from being in tests/ to test/. This makes mocha find the tests automatically.

Update CHANGELOG.rst to use ReStructured Text syntax

We have been surrounding words with single backticks to indicate fixed-width fonts, which is fine for Markdown files, but doesn't have the same effect for ReStructured Text files. Since the changelog is an RST file, I updated the syntax to be compliant with RST.

Additional Notes

I changed the comments on the copyright notice lines to single-line comments so that gulp lint passes properly. Previously it did not consider the notices when they were within multi-line comments.

Coverage Differential

Test coverage went from:

------------------------|----------|----------|----------|----------|-------------------|
File                    |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
------------------------|----------|----------|----------|----------|-------------------|
All files               |    89.11 |    87.13 |    89.83 |    88.81 |                   |
 lib                    |       99 |    99.57 |    98.86 |    98.96 |                   |
  command_factory.js    |      100 |      100 |      100 |      100 |                   |
  format_command.js     |      100 |      100 |      100 |      100 |                   |
  format_data.js        |      100 |      100 |      100 |      100 |                   |
  post_data.js          |      100 |      100 |      100 |      100 |                   |
  slack-messages.js     |      100 |      100 |      100 |      100 |                   |
  slack_monkey_patch.js |    28.57 |       80 |       50 |    28.57 |    17,18,19,40,41 |
  utils.js              |      100 |      100 |      100 |      100 |                   |
 scripts                |    62.96 |    60.55 |    63.33 |    62.96 |                   |
  stackstorm.js         |    62.96 |    60.55 |    63.33 |    62.96 |... 09,410,411,444 |
------------------------|----------|----------|----------|----------|-------------------|

to:

---------------------|----------|----------|----------|----------|-------------------|
File                 |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
---------------------|----------|----------|----------|----------|-------------------|
All files            |    89.35 |    87.13 |    87.76 |    89.05 |                   |
 src                 |    62.16 |    60.55 |    63.33 |    62.16 |                   |
  stackstorm.js      |    62.16 |    60.55 |    63.33 |    62.16 |... 02,403,404,437 |
 src/lib             |      100 |      100 |      100 |      100 |                   |
  command_factory.js |      100 |      100 |      100 |      100 |                   |
  format_command.js  |      100 |      100 |      100 |      100 |                   |
  slack-messages.js  |      100 |      100 |      100 |      100 |                   |
  utils.js           |      100 |      100 |      100 |      100 |                   |
 src/lib/adapters    |    98.72 |      100 |    97.67 |    98.68 |                   |
  botframework.js    |      100 |      100 |      100 |      100 |                   |
  default.js         |      100 |      100 |      100 |      100 |                   |
  hipchat.js         |      100 |      100 |      100 |      100 |                   |
  index.js           |      100 |      100 |      100 |      100 |                   |
  mattermost.js      |      100 |      100 |      100 |      100 |                   |
  matteruser.js      |      100 |      100 |      100 |      100 |                   |
  msteams.js         |      100 |      100 |      100 |      100 |                   |
  rocketchat.js      |      100 |      100 |      100 |      100 |                   |
  slack.js           |    94.37 |      100 |       90 |    94.12 |       48,49,50,55 |
  spark.js           |      100 |      100 |      100 |      100 |                   |
---------------------|----------|----------|----------|----------|-------------------|

Note that the coverage Slack adapter decreased due to moving the Slack monkey patch out of its own module (used to be lib/slack_monkey_patch.js) into the chat provider constructor.

Further Work

The Slack-like adapters can be simplified even further, primarily the Mattermost adapter since we will eventually have end-to-end tests with that.

Rewriting/refactoring the chat adapters individually with Coffeescript or ES6's class and extends is also now possible and drastically easier to implement than before.

Furthermore, the src/stackstorm.js module can still be simplified, similar to how #133 refactors it. This was not done yet because it did have as good of test coverage as the chat adapters, which would necessitate the need for more manual tests, or a lot more effort put into automated tests. However, if that refactoring is done properly, each function should be easier to test individually (eg: unit tests), instead of testing with integration tests (eg: mocking up an environment and/or HTTP responses and running the code singularly from the src/stackstorm.js entrypoint) like we currently do. Ironically, this should hopefully end up reducing the overall amount of tests we need to write, which would also keep our test configuration simple (eg: do most/all of it with Travis tests), make our tests easier to write (unit tests vs. integration test), more reliable (no reliance on st2cicd for end-to-end tests), and faster (no need to run Travis tests and then kick off end-to-end tests on st2cicd).

@blag blag requested a review from arm4b July 17, 2019 05:37
@blag blag requested a review from jinpingh July 17, 2019 05:37
@blag blag force-pushed the refactor-chat-adapters branch from d803d7c to 894b5a1 Compare July 17, 2019 05:38
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work 👍

I would however use adapter dir instead of messaging_handler to keep all the custom Hubot adapters we support.

@blag
Copy link
Contributor Author

blag commented Jul 18, 2019

@armab Updated to use adapters instead of messaging_handler directory, renamed the classes/functions to match, changed all of the requires around, and updated tests.

@blag blag merged commit df94be0 into master Jul 19, 2019
@blag blag deleted the refactor-chat-adapters branch July 19, 2019 04:58
@blag blag mentioned this pull request Jul 19, 2019
@blag blag mentioned this pull request Aug 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants