search

18F / standup-slack-bot

A Slack bot to streamline team standup without disturbing the overall flow of conversation
https://standup-slack-bot.app.cloud.gov/
Other
87 stars 31 forks source link

First unit/integration tests ++ refactoring #128

Closed baccigalupi closed 7 years ago

baccigalupi commented 7 years ago

Unit tests provide a lot more granular testing than acceptance tests (which is what we should be doing with our cucumber suite). Integration tests are the middle ground that cover a collection of modules, but not everything from the database to the ui.

This work started with integration tests and works its way towards unit tests. And it focuses on the module doChannelReport. I first added a couple high level tests to ensure the happy paths kept working. Then I was able to disentangle the code enough to add some unit tests.

One of the bigger confusions that I found in the code in the mixture of synchronous code with async code (as though both were async). Sync code is really easy to unit test, and async code is a ton harder. So separating the two made for easier testing of things like searching through objects in memory and producing reports.

If I were going to continue in this area, I would drop in Bluebird.promisify into the bot methods. Doing it right now is impossible because of the mocking of the bot (see test/support), and also the fact that some of the code never joins from its async work. The tests would be really flakey since there is no guarantee the work would be complete at assertion time.

Anyway, I wanted to stop here since @mgwalker is also doing some refactoring in this area of report/field generation.

The commits should make an interesting read from start to finish. You can see my thinking each step, and always tests pass.

baccigalupi commented 7 years ago

Couple of issues I ran into:

  1. The shrinkwrapping didn't seem right, so I omitted that file. Someone should tell me what I am doing wrong before we merge this.
  2. I didn't integrate the testing command: node_modules/.bin test --recursive because I wasn't sure how to include coverage for cucumber and mocha together. Also, help!
baccigalupi commented 7 years ago

Also, I have had fun before having a half hour multiperson screenhero review on prs like this one. So let me know.

codecov-io commented 7 years ago

Codecov Report

Merging #128 into develop will decrease coverage by -1.2%. The diff coverage is 73.17%.

@@            Coverage Diff             @@
##           develop     #128     +/-   ##
==========================================
- Coverage    86.05%   84.85%   -1.2%     
==========================================
  Files           43       56     +13     
  Lines         1233     1321     +88     
  Branches       175      174      -1     
==========================================
+ Hits          1061     1121     +60     
- Misses         172      200     +28
Impacted Files Coverage Δ
lib/helpers/reports/transformers/index.js 100% <100%> (ø)
lib/helpers/reports/transformers/TodaySearcher.js 100% <100%> (ø)
lib/helpers/reports/generateFields.js 100% <100%> (ø)
lib/helpers/reports/convertStandups.js 100% <100%> (ø)
lib/helpers/reports/forChannel.js 100% <100%> (ø)
lib/helpers/reports/transformers/HeardFrom.js 100% <100%> (ø)
lib/helpers/updateChannelReport.js 20% <20%> (ø)
lib/helpers/reports/forChannelUpdate.js 33.33% <33.33%> (ø)
lib/helpers/reports/transformers/HeaderField.js 56.25% <56.25%> (ø)
lib/helpers/createNewChannelReport.js 60% <60%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update c76f090...58af9fa. Read the comment docs.