Use actionmodel naming for calculators #1546
Conversation
| @@ -4,6 +4,11 @@ | |||
| let(:calculator) { Spree::Calculator::FlatPercentItemTotal.new } | |||
| let(:line_item) { mock_model Spree::LineItem } | |||
|
|
|||
| describe ".description" do | |||
| subject{ described_class.description } | |||
There was a problem hiding this comment.
There's a space missing here before the opening {. Can you add that one space? I know it's lots of commits to go and edit, but interactive rebase makes that pretty straightforward. Read up here about it, it's great. And yes, please force push afterwards, we don't mind for open PRs before they're merged.
d753171 to
1404117
Compare
|
Thanks for your contribution. I don't see any value in testing these descriptions. Especially if we hard core translations inside tests. That only means broken builds if we update a translation. There is no business logic included at all that needed testing. So, from my point of view we should just remove them. |
1404117 to
34ca4cf
Compare
|
@tvdeyen @mamhoff, spoke with @cbrunsdon, he says to keep the tests despite the hard-coding. |
|
Yea, I'm pro leaving the tests in. Since we're testing the output, which is the text string, not the implementation, which we know goes through standard rails i18n. |
|
Testing a random string has no value for me. Why don't we test all other strings then, why exactly these strings? Are they special, and when yes, what makes them special? The only value I see is that we ensure all calculators inherit from Sorry, still not convinced, but I don't resist on removing them. If anybody these value in them, please keep em. I only care about less burdens. |
|
I'd be okay with something like a shared example for calculators that just tested that there was a non-zero length string there |
|
Like that |
| @@ -9,6 +10,11 @@ | |||
| let(:included_in_price) { false } | |||
| subject(:calculator) { Spree::Calculator::DefaultTax.new(calculable: rate ) } | |||
|
|
|||
| describe ".description" do | |||
There was a problem hiding this comment.
You can even put the whole describe block including the subject in the shared example so that you only have to leave it_behaves_like in the calculator specs.
There was a problem hiding this comment.
@gevann I second this thought. In the individual calculator specs, we only want to see something like it_behaves_like "a calculator with a description". The describe block can go into the shared example.
| @@ -0,0 +1,5 @@ | |||
| shared_examples_for 'having a description' do | |||
| it "has a description" do | |||
| expect(subject.size).to be > 0 | |||
There was a problem hiding this comment.
I think expecting a String would be better. Every enumerable has a size method.
| @@ -1,4 +1,5 @@ | |||
| require 'spec_helper' | |||
| require 'spree/testing_support/calculator_helpers' | |||
There was a problem hiding this comment.
The term helper would lead to the impression that this file provides some preparation or tear down related stuff.
Why not call this file shared_calculator_examples or we could add another folder shared_examples and put a calculators file into it.
WDYT @cbrunsdon ?
There was a problem hiding this comment.
I think a new folder would be clearest -- I didn't want to add one myself as I'm new to Solidus, so I just chose a place where I found other shared examples being defined currently.
|
Sorry for being so picky! |
Change model tested from Spree::Calculator::PriceSack to Spree::Calculator::Shipping::PriceSack.
61f6c19 to
aa1bfdf
Compare
|
@tvdeyen I would've been on the same board as you, so not to worry : ), but the way the shared example is implemented is just great! Thanks! |
|
@mamhoff you requested changes that where now made. |
This PR is based on #1538 and closes #1544