Skip to content

Conversation

@Swiftda01
Copy link
Collaborator

  • Controller action
  • Route
  • Logout button
  • Manual testing

expect(route.verb).to eq('DELETE')
end
end
end
Copy link
Owner

Choose a reason for hiding this comment

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

If you do end up adding destroy to the resources array then presumably this spec is no longer needed? I've actually never used routing specs before but I'm guessing they're only needed for custom routes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think the fact that it's a custom route makes it any more or less deserving of tests. These are just unit tests to test a piece of functionality - the functionality here being that the expected route has been generated. Currently our other routes are being tested indirectly through feature specs, but since we don't have one of those for logging out yet I didn't want to add an untested piece of code to the codebase.

I followed the Hanami guides for this: https://guides.hanamirb.org/routing/testing/

Copy link
Owner

Choose a reason for hiding this comment

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

Interesting.. there's a part of me that just thinks this is excessive. It seems to be basically testing that the Hanami routes library is working correctly, rather than testing what we've been doing. If we write a spec for one of these routes then there's no argument really against writing one for all of them. Does that seem worthwhile to you? That's not rhetorical, genuinely interested to hear your thoughts.

Copy link
Collaborator Author

@Swiftda01 Swiftda01 Jul 12, 2020

Choose a reason for hiding this comment

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

Hmm to be honest I don't see this as testing the Hanami routes library, I see it more as checking that the delete sessions route exists for our application and is working as expected.

The reason I added this test is because I was adding functionality to our application which is not covered by an integration-style feature test like our other routes are (as there is no feature that uses this route yet). So I added these specs to ensure that this new unit of code is tested. If I hadn't, then it would be an untested piece of functionality, meaning that for all I know it could be broken or not behaving as expected - or if it was changed or removed then there would be no failing tests to reflect this.

Having individual unit tests for routes also means that if you change or remove the feature test, then you can rest assured that this unit of code is still properly tested in isolation. We already follow this principle in other areas of the codebase such as having a test for web/controllers/users/new.rb when strictly speaking it is already covered by the create user feature specs.

I personally I like the belt and braces approach of having both unit tests and overall feature tests to ensure the test coverage is robust, but if you feel it's excessive in this instance then I'm happy to leave these tests out for routes.

This was my thinking behind adding these tests, but obviously I'm still very new to Hanami so interested to hear your thoughts

@Swiftda01 Swiftda01 marked this pull request as ready for review August 13, 2020 20:23
@Swiftda01 Swiftda01 requested a review from DannySantos August 13, 2020 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants