-
Notifications
You must be signed in to change notification settings - Fork 12
Added bats integration tests to verify that users can send mail via ssmtp #11
base: master
Are you sure you want to change the base?
Added bats integration tests to verify that users can send mail via ssmtp #11
Conversation
Looks like bats runs as root via test-kitchen, so this only tests email sent by root
Install pre-release mailtrap gem if 0.2.3 is not found on rubygems Wait max of 5 seconds for mailtrap to be ready
- Dynamically find ssmtp executable path (not the same on all distros) - Unset the options that the bats test runner sets for us during setup... we definitely don't want the errexit option during the setup phase
- Reinstate mailtrap teardown function now that race condition test works - Send subject header in body since ssmtp does not support -s - Make sure to get groups of vagrant user so we don't break sudo by adding them to only the groups root has (Fixes second run of verify on VM `kitchen test -d never && kitchen verify`) - Hopefully make bats tests more user agnostic (should work as long as they run as root or a user who has passwordless sudo) - Fix bats/bash script OS portability on CentOS 5.8: - Check for any executable bit set in find_mailtrap_bin - Run `which ssmtp` as root to find ssmtp binary on distros that don't include sbin dirs on normal user's PATH - Run usermod as root to ensure it works when sbin is not in user's PATH
Hi Thanks for the PRs. The two PRs are all on the same branch which means they can not pull them in separately. Was that the intent? |
Yes. Since the other PR was based off of this one, the patches depend on the prior commits to apply cleanly |
Okay. This means that I there is a lot of code to accept all at once. It will take me quite a bit more time to ensure we can accept it all. If there are things we may want to discuss it may be hard to make adjustments. I'll see what I can do, but just so you know. Having each of them come in as separate PRs on separate branches will increase my ability to accept your changes within a reasonable amount of time. |
ok, understood. I'll keep that in mind in the future |
@trinitronx I tried to merge this in today, but a dependency on a patched version of mailtrap gem is hard to take into the main line. We have been using minitest for the cookbook and the plan was to add more tests for that. I definitely see the appeal to bats, but over time it is easy to outgrow the simple bats tests. While running this I got the following error:
I will see what I can salvage from the rest of your work, but ideally we would have these tests depend on a released version of gems and pass tests. Let me know what you would like to do. |
@svanzoest The dependency on the updated version of mailtrap is indeed necessary for the tests to work, as I had to fork and update the gem to work well with testing ssmtp. I've got an open pull request on mmower/mailtrap#7 but not sure when or if that will be merged or not. I agree that bats tests with the development gem are not ideal. If that ever gets merged and released, perhaps the gem could be installed via chef and used for testing in minitest. The bats test setup and teardown took a while to get working, and seems too complicated when compared to the actual ssmtp test commands to verify that it works. |
@trinitronx looks like mmower/mailtrap#7 has merged. Let's get this pulled in :-) |
request off of trinitronx/mailtrap@develop