Conversation
mattstauffer
left a comment
mattstauffer
left a comment
There was a problem hiding this comment.
I'm not going to lie, I only skimmed the tests, because there's so much there. LMK if you'd like me to really go through them more in depth. Great work here, really an incredible amount of work!
|
|
||
| public function subscribe($events) | ||
| { | ||
| if (empty(config('app.slack_endpoint')) || App::environment('testing')) { |
There was a problem hiding this comment.
Is this no longer a concern, where this would throw an error in tests and if someone hadn't set up Slack?
There was a problem hiding this comment.
This is handled by calling Notification::fake() in the base TestCase. Not returning early in the subscriber then allows for testing that a notification was sent via a call to Notification::assertSentToTightenSlack(NewConference::class);.
There was a problem hiding this comment.
OK, that makes sense for the testing environment check, but what about people installing this locally who don't have a Slack endpoint setup?
There was a problem hiding this comment.
I hadn't thought of that, but I think I have come to a good solution. This commit overrides the slack notification channel when the endpoint isn't set, and will log the notification instead of sending it.
The above allows the test suite to cover the whole feature flow without affecting users running the app locally. This also has the added benefit of being able to inspect the notification in the log.
|
@andrewmile Note.. i asked a bunch of "are we just not using this anymore" questions, but then saw your note about "Inaccessible code paths and unused methods have been removed"... so i guess we're not using all that code lol 🤷 |
Co-authored-by: Matt Stauffer <mattstauffer@users.noreply.github.com>
2 participants
This PR increases Symposium's test coverage to 100%. This adds a safety net, both for future PRs and for introducing PHPStan into the codebase. Outside of the additional tests themselves, the following notable changes were made:
TweetImportantCFPDatescommand has been removed@codeCoverageIgnoreannotation