Split up and test Package and PackageItem #435

kmcphillips · 2016-12-06T03:17:51Z

What's the deal with `Package` and `PackageItem`

Problem

These classes are a bit of a mess. The APIs are not clear, they don't do what you think they'd do, they have implicit behaviour, they're mostly untested, and we have no confidence in changing them.

Solution

Split the class out into its own file. Test the specs as they exist.

There are even a bunch of cases where calling them with valid APIs have bugs and they raise. I'm not changing any behaviour. Just asserting the raises and skipping the tests. If the tests stop raising they'll then fail.

This lets us pull out Quantified and replace with measured as in #434. This is really just a first step in that refactor.

Note: I wrote so many tests you may have to click View Diff because the last file with 400+ lines is collapsed.

@jonathankwok @mdking @thegedge

kmcphillips · 2016-12-06T03:22:05Z

test/unit/package_test.rb

-                      )
-  end
-
-  def test_accessors


This test was incomplete and wasn't really testing everything clearly. Replaced it with many other tests testing each feature explicitly.

kmcphillips · 2016-12-06T03:22:49Z

lib/active_shipping/package_item.rb

@@ -0,0 +1,72 @@
+module ActiveShipping #:nodoc:
+  class PackageItem


Nothing in here changed. Just moved.

kmcphillips · 2016-12-06T03:23:15Z

test/unit/package_item_test.rb

+  end
+
+  def test_initialize_weight_mass_object
+    begin


This is one of the tests that asserts the broken behaviour as a skip, but then will fail if it is no longer broken.

kmcphillips · 2016-12-06T03:40:53Z

A first step toward #427 in 2.0

thegedge

Great test suite! A few notes, but mostly I want us to use tighter deltas (to increase trust in this library) and no bare asserts/refutes.

thegedge · 2016-12-06T14:53:50Z

test/unit/package_item_test.rb

+    assert_equal @weight, @item.grams
+  end
+
+  def test_initialize_negative_quantity


I think test names are better when they outline the expectation, not the thing they're testing (both is fine too). Some examples:

test_initialize_with_negative_quantity_should_default_to_one

test_initialize_forces_positive_quantity

It becomes a little verbose, but also very clear what's happening in here.

I really dislike the def test_ DSL here. It's part of what makes minitest so hard to work with.

I'll be coming back and naming all the tests with test "#method_name action conditions" do as part of 2.0. So I'm not really worried about the naming here.

thegedge · 2016-12-06T14:57:06Z

test/unit/package_item_test.rb

+    refute_equal @weight, @item.ounces
+  end
+
+  def test_initialize_weight_does_not_accept_strings


Just for noting: I don't think this is a good behaviour. Either accepting both or raising would be preferred, but either way I'll be surprised to get metric here.

All these tests test inconsistent behaviour. We are in agreement.

thegedge · 2016-12-06T15:02:16Z

test/unit/package_item_test.rb

+
+  def test_grams_value
+    assert_equal 100, @item.grams
+  end


This seems inconsistent. We have test_initialize_with_all_attributes and then this one. We're testing the same behaviour (state on initialization), but across two tests.

This is likely a difference between classical unit testing and behaviour-driven testing, where the former focuses more on individual methods, but I'm more for the latter. What are your thoughts?

Again. All part of a bad interface. In that the grams method is both an accessor and performing logic. There is no way to test these pieces in isolation. So I have opted to be a bit redundant, and test what is available, and all edges of the public interface. However awkward.

I don't think this is part of a bad interface, but simply a mixture of testing paradigms. There's a paradigm where you focus on individual methods and the inputs/outputs of those. Then there's behavioural testing where you have expected behaviours of your classes and test those, which often means testing across methods / accessors / readers / etc. I'm a big fan of more of the latter, but it's totally fine to mix and match.

What's confusing to me, and the reason for this comment, is that we have test_initialize_with_all_attributes test, which tests the state of things after initialization, and that's exactly what this test is also doing. We've taken a behaviour (state immediately after initialization) and split it across the wrong boundary (attr_reader / def foo). Whether there's logic or not happening in those should be irrelevant to clients of the object. Also, there's nothing stopping us from "upgrading" one of the attr_readers to a manual def attr; ...stuff...; end in the future.

Sorry for the wordiness, but hopefully that cleared up my stance :)

P.S. I don't want to block on shipping because of this, but I do believe this would make for a higher quality and easier to understand test suite.

thegedge · 2016-12-06T15:04:41Z

test/unit/package_item_test.rb

+  def test_ounces_converts
+    @item = PackageItem.new(@name, @weight, @value, @quantity, @options.merge(units: :metric))
+
+    assert_in_delta 3.5, @item.ounces, 1


Again, large delta.

thegedge · 2016-12-06T15:06:24Z

test/unit/package_item_test.rb

+  def test_pounds_converts
+    @item = PackageItem.new(@name, 1000, @value, @quantity, @options.merge(units: :metric))
+
+    assert_in_delta 2.2, @item.pounds, 1


I'll just let you find all of these :)

thegedge · 2016-12-06T15:10:07Z

test/unit/package_item_test.rb

+      flunk "This code path is broken but passed unexpectedly"
+    rescue NoMethodError
+      skip "This code path is broken"
+    end


A couple of things:

Although this is slightly different, I wonder would assert_raises(NameError) { ... } basically do the same thing for us? I guess it's not good at conveying what we want to convey here, since it says "we expect this to happen".

If we want this exactly, it's used enough that maybe a helper function in TestHelper would be nice? broken_with(NameError) { ... }, or whatever name you please. I'm fine keeping these here too, since they'll hopefully disappear soon!

We decided against 1. But 2, that's what this does. It doesn't do the indirection through a helper, but it definitely is explicit in that we expect a failure, then skip the test.

Yeah, I'm game for keeping this. Like I mentioned, this will hopefully disappear quickly :)

thegedge · 2016-12-06T15:11:26Z

test/unit/package_item_test.rb

+  def test_grams_converts
+    @item = PackageItem.new(@name, 100, @value, @quantity, @options.merge(units: :imperial))
+
+    assert_in_delta 2834.9, @item.grams, 1


1 is a very large window of error, which makes me lose trust in the library. I'd tighten up the error bounds, e.g., assert_in_delta 2834.95, @item.grams, 0.01.

thegedge · 2016-12-06T15:14:15Z

test/unit/package_test.rb

+  end
+
+  def test_unpackaged
+    assert Package.new(@weight, @dimensions, unpackaged: true).unpackaged?


Prefer avoiding a bare assert / refute (bare meaning without an error message) since the failed assertion message is useless. assert_predicate Package.new(...), :unpackaged? for this, and anywhere else here.

thegedge · 2016-12-06T15:16:28Z

test/unit/package_test.rb

+    assert_equal @dimensions[0], @imperial_package.inches(:height)
+    assert_equal @dimensions[0], @imperial_package.inches(:depth)
+    assert_equal @dimensions[0], @imperial_package.inches(:high)
+    assert_equal @dimensions[0], @imperial_package.inches(:deep)


Just for noting: I'm personally not a fan of this behaviour. It's adding unnecessary complexity / surface area to this library. Are they useful? My own opinion is that they aren't, and that we should let clients decide if they want to alias things.

thegedge · 2016-12-06T15:18:57Z

test/unit/package_test.rb

+    assert_equal 12, Package.cents_from(12)
+  end
+
+  def test_cents_from_nonsense


So much 😂 for this name. I think I'd be on the floor if it were noncents...

jonathankwok

jonathankwok · 2016-12-06T15:20:33Z

test/unit/package_test.rb

+  def test_inches_x
+    assert_equal @dimensions[2], @imperial_package.inches(:x)
+    assert_equal @dimensions[2], @imperial_package.inches(:max)
+    assert_equal @dimensions[2], @imperial_package.inches(:length)


I had no idea this was possible.

Let's make it impossible ok? And make a more normal looking API.

jonathankwok · 2016-12-06T15:24:39Z

test/unit/package_test.rb

+    assert_in_delta 17.2, @package.centimetres(:girth), 1
+    assert_in_delta 17.2, @package.centimetres(:around), 1
+    assert_in_delta 17.2, @package.centimetres(:circumference), 1
+


Split up and test Package and PackageItem

kmcphillips commented Dec 6, 2016

View reviewed changes

thegedge suggested changes Dec 6, 2016

View reviewed changes

jonathankwok approved these changes Dec 6, 2016

View reviewed changes

Split up and test Package and PackageItem

bb56c2a

kmcphillips force-pushed the package-refactor-test branch from d0e2c6e to bb56c2a Compare December 6, 2016 19:48

mdking approved these changes Dec 6, 2016

View reviewed changes

kmcphillips merged commit aaca900 into master Dec 7, 2016

kmcphillips deployed to rubygems December 7, 2016 16:24 Active

kmcphillips had a problem deploying to rubygems-1-9-stable December 15, 2016 16:18 Failure

jonathankwok had a problem deploying to rubygems-1-9-stable January 30, 2017 18:45 Failure

jonathankwok had a problem deploying to rubygems-1-9-stable January 30, 2017 18:51 Failure

kmcphillips deployed to rubygems-1-9-stable January 30, 2017 18:52 Active

maartenvg deleted the package-refactor-test branch September 18, 2017 15:54

maartenvg pushed a commit that referenced this pull request Nov 9, 2017

Merge pull request #435 from Shopify/package-refactor-test

cbd6597

Split up and test Package and PackageItem

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split up and test Package and PackageItem #435

Split up and test Package and PackageItem #435

kmcphillips commented Dec 6, 2016

kmcphillips Dec 6, 2016

kmcphillips Dec 6, 2016

kmcphillips Dec 6, 2016

kmcphillips commented Dec 6, 2016

thegedge left a comment

thegedge Dec 6, 2016

kmcphillips Dec 6, 2016

thegedge Dec 6, 2016

kmcphillips Dec 6, 2016

thegedge Dec 6, 2016

kmcphillips Dec 6, 2016

thegedge Dec 6, 2016 •

edited

Loading

thegedge Dec 6, 2016

thegedge Dec 6, 2016

thegedge Dec 6, 2016

thegedge Dec 6, 2016 •

edited

Loading

kmcphillips Dec 6, 2016

thegedge Dec 6, 2016

thegedge Dec 6, 2016

thegedge Dec 6, 2016 •

edited

Loading

thegedge Dec 6, 2016

thegedge Dec 6, 2016

jonathankwok left a comment

jonathankwok Dec 6, 2016

kmcphillips Dec 6, 2016

jonathankwok Dec 6, 2016

		@@ -0,0 +1,72 @@
		module ActiveShipping #:nodoc:
		class PackageItem

Split up and test Package and PackageItem #435

Split up and test Package and PackageItem #435

Conversation

kmcphillips commented Dec 6, 2016

What's the deal with Package and PackageItem

Problem

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kmcphillips commented Dec 6, 2016

thegedge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegedge Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegedge Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thegedge Dec 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathankwok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

What's the deal with `Package` and `PackageItem`

thegedge Dec 6, 2016 •

edited

Loading

thegedge Dec 6, 2016 •

edited

Loading

thegedge Dec 6, 2016 •

edited

Loading