WIP Add complete unit tests to the weight_limit spec #10
Conversation
fredericboivin
force-pushed
the
weight_tests
branch
4 times, most recently
from
d5c7399 to
d7330ec
Compare
Begin adding unit tests to document the behavior of the various functions related to preparing packages and calculating weight.
| @@ -91,6 +101,171 @@ def build_inventory_unit(variant, order) | |||
| Spree::ActiveShipping::Config.set(:default_weight => 1) | |||
| end | |||
|
|
|||
| describe ".valid_weight_for_package?" do | |||
There was a problem hiding this comment.
Usually we do not test private methods, they should be tested as part of the behavior of the public method that they are called from. In this particular case the valid_weight_for_package? helper method should be tested as part of the expected behavior for the ActiveShipping::Base#available?. The tests for that should probably live in a spec/models/spree/calculator/shipping/active_shipping/base_spec.rb file however. Currently there are some tests for that in spec/models/active_shipping_calculator_spec.rb but we should probably move those in the correct place.
There was a problem hiding this comment.
(Adressing both comments)
I understand your point, though I have some concerns : there are multiple levels of indirection between all the functions being called and the behavior is not consistent (sometimes it's rescued, sometimes it's not ; some valid paths are crashing because of bugs in the code related to nil weight or max weight limit, some paths are not used at all), so it is hard to test all the possible edge cases from the compute or similar higher level methods, and testing each of those invidually is helpful in finding the underlying bugs.
Maybe those tests does not need to be merged but I think they are really useful in understanding the messy parts of the code and documenting current behavior before it can be extracted.
As for the location of the tests file, you're also right. I didn't concern myself with it too much because I was planning on merging the two files once I was finished, and the namespace should be corrected.
Maybe I can just extract it into a new class for now without changing any of the code? It will be still be easier to test and both specs will be leaner and more concise.
Either way, thanks for the feedback.
There was a problem hiding this comment.
I agree with you on all of this. I think if we can extract all of the package weight stuff into a class without modifying the logic and adding the tests you have for that. We'll be in a much better place and avoid testing these private methods.
Let me know if you want to chat more about what that might look like. Thanks again for going through and figuring out all this convoluted logic ❤️
There was a problem hiding this comment.
Working on it !
|
@dangerdogz If you want, pull the first commit in a separate PR and we can run transpec on all the specs and merge that separately while you are working on the weight refactor/specs. |
|
Won't be used since we went another way but I documented some bugs in there so leaving it around until I can fix them for sure (related to issue #28 ) |
fredericboivin
force-pushed
the
weight_tests
branch
from
d7330ec to
f3ab17d
Compare
The weight limit spec and the associated methods contains a lot of code who looks unreachable or that we're not sure why or how it works (or even if we need it)
The plan would be to remove, refactor or extract some of those parts but they are virtually untested so I've begun adding unit tests for each individual methods.
This is a work in progress, comments and suggestions are very welcome.
I also used transpec to update the syntax.