[numbers-fraction] Code duplication between FractionTest and BigFractionTest

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

[numbers-fraction] Code duplication between FractionTest and BigFractionTest

Heinrich Bohne
An awful lot of code is duplicated between FractionTest and
BigFractionTest. Often, the test cases in the two classes only differ in
the types they use (e.g. Fraction vs. BigFraction), but the actual
values the tests use are the same.

I think this could be mitigated by adding a new class that stores the
values for these common test cases, and the classes FractionTest and
BigFractionTest retrieve the values from this class and only implement
the test patterns.

I created a draft here:
https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f

Any opinions on this?

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Alex Herbert


> On 20 Jun 2019, at 00:54, Heinrich Bohne <[hidden email]> wrote:
>
> An awful lot of code is duplicated between FractionTest and
> BigFractionTest. Often, the test cases in the two classes only differ in
> the types they use (e.g. Fraction vs. BigFraction), but the actual
> values the tests use are the same.
>
> I think this could be mitigated by adding a new class that stores the
> values for these common test cases, and the classes FractionTest and
> BigFractionTest retrieve the values from this class and only implement
> the test patterns.
>
> I created a draft here:
> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
>
> Any opinions on this?

1. BigFraction should work the same way as Fraction when the numbers are the same

So collecting the common tests together makes sense. The change in the PR looks good.

2. BigFraction should work with numbers that cannot be handled by Fraction

A quick looks shows that the BigFractionTest does have test cases for very large numbers. However the add, subtract, divide and multiply tests and a few others just use values that would work with Fraction. Possibly these can be moved to a shared common tests location too.

Then variants added using BigInteger arguments just to make sure the Big part of BigFraction is working.

>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Heinrich Bohne
> A quick looks shows that the BigFractionTest does have test cases for very large numbers. However the add, subtract, divide and multiply tests and a few others just use values that would work with Fraction. Possibly these can be moved to a shared common tests location too.

That's what I was thinking too – the draft was by no means intended to be complete, I just created it to give a general idea of how I would go about implementing this. I'll work some more on it before I create an actual pull request.


On 6/20/19 10:40 AM, Alex Herbert wrote:

>
>> On 20 Jun 2019, at 00:54, Heinrich Bohne <[hidden email]> wrote:
>>
>> An awful lot of code is duplicated between FractionTest and
>> BigFractionTest. Often, the test cases in the two classes only differ in
>> the types they use (e.g. Fraction vs. BigFraction), but the actual
>> values the tests use are the same.
>>
>> I think this could be mitigated by adding a new class that stores the
>> values for these common test cases, and the classes FractionTest and
>> BigFractionTest retrieve the values from this class and only implement
>> the test patterns.
>>
>> I created a draft here:
>> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
>>
>> Any opinions on this?
> 1. BigFraction should work the same way as Fraction when the numbers are the same
>
> So collecting the common tests together makes sense. The change in the PR looks good.
>
> 2. BigFraction should work with numbers that cannot be handled by Fraction
>
> A quick looks shows that the BigFractionTest does have test cases for very large numbers. However the add, subtract, divide and multiply tests and a few others just use values that would work with Fraction. Possibly these can be moved to a shared common tests location too.
>
> Then variants added using BigInteger arguments just to make sure the Big part of BigFraction is working.
>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Eric Barnhill
Sorry for the slow reply, I thought I sent this yesterday.

I agree from a code architecture standpoint such a refactoring makes sense.
However from the perspective of unit tests it makes it no longer a unit
test.

IIUC it's best practice for a unit test that all context be within the
test. If additional context is required it fails to meet the definition of
a unit test and is instead an integration test,  and the function being
tested may require rethinking.

This results in unit tests often being clunkily and awkwardly coded, but I
think it is the way they are typically written and it has its reasons.

 So I am +0 .

On Thu, Jun 20, 2019, 02:01 Heinrich Bohne <[hidden email]> wrote:

> > A quick looks shows that the BigFractionTest does have test cases for
> very large numbers. However the add, subtract, divide and multiply tests
> and a few others just use values that would work with Fraction. Possibly
> these can be moved to a shared common tests location too.
>
> That's what I was thinking too – the draft was by no means intended to be
> complete, I just created it to give a general idea of how I would go about
> implementing this. I'll work some more on it before I create an actual pull
> request.
>
>
> On 6/20/19 10:40 AM, Alex Herbert wrote:
> >
> >> On 20 Jun 2019, at 00:54, Heinrich Bohne <[hidden email]> wrote:
> >>
> >> An awful lot of code is duplicated between FractionTest and
> >> BigFractionTest. Often, the test cases in the two classes only differ in
> >> the types they use (e.g. Fraction vs. BigFraction), but the actual
> >> values the tests use are the same.
> >>
> >> I think this could be mitigated by adding a new class that stores the
> >> values for these common test cases, and the classes FractionTest and
> >> BigFractionTest retrieve the values from this class and only implement
> >> the test patterns.
> >>
> >> I created a draft here:
> >>
> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
> >>
> >> Any opinions on this?
> > 1. BigFraction should work the same way as Fraction when the numbers are
> the same
> >
> > So collecting the common tests together makes sense. The change in the
> PR looks good.
> >
> > 2. BigFraction should work with numbers that cannot be handled by
> Fraction
> >
> > A quick looks shows that the BigFractionTest does have test cases for
> very large numbers. However the add, subtract, divide and multiply tests
> and a few others just use values that would work with Fraction. Possibly
> these can be moved to a shared common tests location too.
> >
> > Then variants added using BigInteger arguments just to make sure the Big
> part of BigFraction is working.
> >
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: [hidden email]
> >> For additional commands, e-mail: [hidden email]
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: [hidden email]
> > For additional commands, e-mail: [hidden email]
> >
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>
Reply | Threaded
Open this post in threaded view
|

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Alex Herbert
On 20/06/2019 16:07, Eric Barnhill wrote:
> Sorry for the slow reply, I thought I sent this yesterday.
>
> I agree from a code architecture standpoint such a refactoring makes sense.
> However from the perspective of unit tests it makes it no longer a unit
> test.
It is still testing a unit. Just the test code is spread out over
multiple files so it can be reused.
>
> IIUC it's best practice for a unit test that all context be within the
> test.

If the purpose of the test is to ensure two pieces of code do the same
thing then perhaps a rewrite as a parameterized test. Unfortunately
BigFraction and Fraction do not share a common interface that the test
is checking. So a parameterized test is not as simple as passing in an
instance of one or the other (or a reference to the factory constructor
method). Putting all the test cases in a single place rather than
duplicating them in two files seems like the most maintainable going
forward.

> If additional context is required it fails to meet the definition of
> a unit test and is instead an integration test,  and the function being
> tested may require rethinking.

Depends what you define as a unit test. I'd say the unit was BigFraction
or Fraction. An integration test is something that must be tested with
coherant components working together to provide functionality. You are
not doing that.

I'd say that if you executed the all the tests in the Test file and it
achieves 100% coverage of the class it is aimed at then it is a unit
test. Even if your Test file uses code from other places.

In this instance the code from other places amounts to a list of
arguments and expected results. Duplicating this data just to make the
Test file standalone is less maintainable.

>
> This results in unit tests often being clunkily and awkwardly coded, but I
> think it is the way they are typically written and it has its reasons.
>
>   So I am +0 .
>
> On Thu, Jun 20, 2019, 02:01 Heinrich Bohne <[hidden email]> wrote:
>
>>> A quick looks shows that the BigFractionTest does have test cases for
>> very large numbers. However the add, subtract, divide and multiply tests
>> and a few others just use values that would work with Fraction. Possibly
>> these can be moved to a shared common tests location too.
>>
>> That's what I was thinking too – the draft was by no means intended to be
>> complete, I just created it to give a general idea of how I would go about
>> implementing this. I'll work some more on it before I create an actual pull
>> request.
>>
>>
>> On 6/20/19 10:40 AM, Alex Herbert wrote:
>>>> On 20 Jun 2019, at 00:54, Heinrich Bohne <[hidden email]> wrote:
>>>>
>>>> An awful lot of code is duplicated between FractionTest and
>>>> BigFractionTest. Often, the test cases in the two classes only differ in
>>>> the types they use (e.g. Fraction vs. BigFraction), but the actual
>>>> values the tests use are the same.
>>>>
>>>> I think this could be mitigated by adding a new class that stores the
>>>> values for these common test cases, and the classes FractionTest and
>>>> BigFractionTest retrieve the values from this class and only implement
>>>> the test patterns.
>>>>
>>>> I created a draft here:
>>>>
>> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
>>>> Any opinions on this?
>>> 1. BigFraction should work the same way as Fraction when the numbers are
>> the same
>>> So collecting the common tests together makes sense. The change in the
>> PR looks good.
>>> 2. BigFraction should work with numbers that cannot be handled by
>> Fraction
>>> A quick looks shows that the BigFractionTest does have test cases for
>> very large numbers. However the add, subtract, divide and multiply tests
>> and a few others just use values that would work with Fraction. Possibly
>> these can be moved to a shared common tests location too.
>>> Then variants added using BigInteger arguments just to make sure the Big
>> part of BigFraction is working.
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Heinrich Bohne
In reply to this post by Eric Barnhill
Hello Eric,

I'm not sure if I understand what you mean by "context" when you say
that all context has to be within the unit test.Do you mean that the
test should not rely on the functionality of other
modules/methods/"units" than the one to be tested? If so, I agree with
you, but I don't think that this would be the case in this scenario.

The test methods in FractionTest would still only test the functionality
of Fraction, and those in BigFractionTest only that of BigFraction. It's
just that, in cases where similar/analogous functionality is tested,
they don't "come up" with test cases, i.e. combinations of input and
expected output values, themselves, but rather draw these from an
outside source that just happens to be queried by unit tests of a
different, entirely unrelated class as well.

The class FractionTest does not care that the class BigFractionTest
retrieves some of its test cases from the same source as FractionTest
does and vice versa. Likewise, the hypothetical class CommonTestCases
does not care what FractionTest or BigFractionTest do with the test
cases it provides, or if they use them at all. What matters is that
FractionTest and BigFractionTest each take responsibility for ensuring
that the test cases provided by CommonTestCases are applicable to the
functionality they want to test. Of course, this requires that
CommonTestCases clearly documents the test cases it provides. If, for
example, CommonTestCases were to provide test cases that are completely
useless to both FractionTest and BigFractionTest, then it would be the
responsibility of FractionTest and BigFractionTest to disregard them.

So the tests performed by FractionTest and BigFractionTest would not
rely on any functionality outside the unit they test, except for the
validity of the test cases. But erroneous test cases can happen
regardless of whether they are declared in the test class itself or outside.


On 6/20/19 5:07 PM, Eric Barnhill wrote:

> Sorry for the slow reply, I thought I sent this yesterday.
>
> I agree from a code architecture standpoint such a refactoring makes sense.
> However from the perspective of unit tests it makes it no longer a unit
> test.
>
> IIUC it's best practice for a unit test that all context be within the
> test. If additional context is required it fails to meet the definition of
> a unit test and is instead an integration test,  and the function being
> tested may require rethinking.
>
> This results in unit tests often being clunkily and awkwardly coded, but I
> think it is the way they are typically written and it has its reasons.
>
>   So I am +0 .
>
> On Thu, Jun 20, 2019, 02:01 Heinrich Bohne <[hidden email]> wrote:
>
>>> A quick looks shows that the BigFractionTest does have test cases for
>> very large numbers. However the add, subtract, divide and multiply tests
>> and a few others just use values that would work with Fraction. Possibly
>> these can be moved to a shared common tests location too.
>>
>> That's what I was thinking too – the draft was by no means intended to be
>> complete, I just created it to give a general idea of how I would go about
>> implementing this. I'll work some more on it before I create an actual pull
>> request.
>>
>>
>> On 6/20/19 10:40 AM, Alex Herbert wrote:
>>>> On 20 Jun 2019, at 00:54, Heinrich Bohne <[hidden email]> wrote:
>>>>
>>>> An awful lot of code is duplicated between FractionTest and
>>>> BigFractionTest. Often, the test cases in the two classes only differ in
>>>> the types they use (e.g. Fraction vs. BigFraction), but the actual
>>>> values the tests use are the same.
>>>>
>>>> I think this could be mitigated by adding a new class that stores the
>>>> values for these common test cases, and the classes FractionTest and
>>>> BigFractionTest retrieve the values from this class and only implement
>>>> the test patterns.
>>>>
>>>> I created a draft here:
>>>>
>> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
>>>> Any opinions on this?
>>> 1. BigFraction should work the same way as Fraction when the numbers are
>> the same
>>> So collecting the common tests together makes sense. The change in the
>> PR looks good.
>>> 2. BigFraction should work with numbers that cannot be handled by
>> Fraction
>>> A quick looks shows that the BigFractionTest does have test cases for
>> very large numbers. However the add, subtract, divide and multiply tests
>> and a few others just use values that would work with Fraction. Possibly
>> these can be moved to a shared common tests location too.
>>> Then variants added using BigInteger arguments just to make sure the Big
>> part of BigFraction is working.
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: [hidden email]
>> For additional commands, e-mail: [hidden email]
>>
>>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Heinrich Bohne
By the way, I've worked a bit on the draft in the meantime and pushed
the changes I've made so far, in case anyone is interested in
(re-)viewing them. Here's the link to the branch again:
https://github.com/Schamschi/commons-numbers/tree/FractionCommonTestCases

On 6/20/19 6:13 PM, Heinrich Bohne wrote:

> Hello Eric,
>
> I'm not sure if I understand what you mean by "context" when you say
> that all context has to be within the unit test.Do you mean that the
> test should not rely on the functionality of other
> modules/methods/"units" than the one to be tested? If so, I agree with
> you, but I don't think that this would be the case in this scenario.
>
> The test methods in FractionTest would still only test the functionality
> of Fraction, and those in BigFractionTest only that of BigFraction. It's
> just that, in cases where similar/analogous functionality is tested,
> they don't "come up" with test cases, i.e. combinations of input and
> expected output values, themselves, but rather draw these from an
> outside source that just happens to be queried by unit tests of a
> different, entirely unrelated class as well.
>
> The class FractionTest does not care that the class BigFractionTest
> retrieves some of its test cases from the same source as FractionTest
> does and vice versa. Likewise, the hypothetical class CommonTestCases
> does not care what FractionTest or BigFractionTest do with the test
> cases it provides, or if they use them at all. What matters is that
> FractionTest and BigFractionTest each take responsibility for ensuring
> that the test cases provided by CommonTestCases are applicable to the
> functionality they want to test. Of course, this requires that
> CommonTestCases clearly documents the test cases it provides. If, for
> example, CommonTestCases were to provide test cases that are completely
> useless to both FractionTest and BigFractionTest, then it would be the
> responsibility of FractionTest and BigFractionTest to disregard them.
>
> So the tests performed by FractionTest and BigFractionTest would not
> rely on any functionality outside the unit they test, except for the
> validity of the test cases. But erroneous test cases can happen
> regardless of whether they are declared in the test class itself or
> outside.
>
>
> On 6/20/19 5:07 PM, Eric Barnhill wrote:
>> Sorry for the slow reply, I thought I sent this yesterday.
>>
>> I agree from a code architecture standpoint such a refactoring makes
>> sense.
>> However from the perspective of unit tests it makes it no longer a unit
>> test.
>>
>> IIUC it's best practice for a unit test that all context be within the
>> test. If additional context is required it fails to meet the
>> definition of
>> a unit test and is instead an integration test,  and the function being
>> tested may require rethinking.
>>
>> This results in unit tests often being clunkily and awkwardly coded,
>> but I
>> think it is the way they are typically written and it has its reasons.
>>
>>   So I am +0 .
>>
>> On Thu, Jun 20, 2019, 02:01 Heinrich Bohne <[hidden email]>
>> wrote:
>>
>>>> A quick looks shows that the BigFractionTest does have test cases for
>>> very large numbers. However the add, subtract, divide and multiply
>>> tests
>>> and a few others just use values that would work with Fraction.
>>> Possibly
>>> these can be moved to a shared common tests location too.
>>>
>>> That's what I was thinking too – the draft was by no means intended
>>> to be
>>> complete, I just created it to give a general idea of how I would go
>>> about
>>> implementing this. I'll work some more on it before I create an
>>> actual pull
>>> request.
>>>
>>>
>>> On 6/20/19 10:40 AM, Alex Herbert wrote:
>>>>> On 20 Jun 2019, at 00:54, Heinrich Bohne <[hidden email]>
>>>>> wrote:
>>>>>
>>>>> An awful lot of code is duplicated between FractionTest and
>>>>> BigFractionTest. Often, the test cases in the two classes only
>>>>> differ in
>>>>> the types they use (e.g. Fraction vs. BigFraction), but the actual
>>>>> values the tests use are the same.
>>>>>
>>>>> I think this could be mitigated by adding a new class that stores the
>>>>> values for these common test cases, and the classes FractionTest and
>>>>> BigFractionTest retrieve the values from this class and only
>>>>> implement
>>>>> the test patterns.
>>>>>
>>>>> I created a draft here:
>>>>>
>>> https://github.com/Schamschi/commons-numbers/commit/53906afd991cd190f1a05beb0952a40ae6c6ea3f
>>>
>>>>> Any opinions on this?
>>>> 1. BigFraction should work the same way as Fraction when the
>>>> numbers are
>>> the same
>>>> So collecting the common tests together makes sense. The change in the
>>> PR looks good.
>>>> 2. BigFraction should work with numbers that cannot be handled by
>>> Fraction
>>>> A quick looks shows that the BigFractionTest does have test cases for
>>> very large numbers. However the add, subtract, divide and multiply
>>> tests
>>> and a few others just use values that would work with Fraction.
>>> Possibly
>>> these can be moved to a shared common tests location too.
>>>> Then variants added using BigInteger arguments just to make sure
>>>> the Big
>>> part of BigFraction is working.
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: [hidden email]
>>>>> For additional commands, e-mail: [hidden email]
>>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: [hidden email]
>>>> For additional commands, e-mail: [hidden email]
>>>>
>>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: [hidden email]
>>> For additional commands, e-mail: [hidden email]
>>>
>>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [hidden email]
> For additional commands, e-mail: [hidden email]
>
>


---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Eric Barnhill
In reply to this post by Alex Herbert
>
> > If additional context is required it fails to meet the definition of
> > a unit test and is instead an integration test,  and the function being
> > tested may require rethinking.
>
> Depends what you define as a unit test. I'd say the unit was BigFraction
> or Fraction. An integration test is something that must be tested with
> coherant components working together to provide functionality. You are
> not doing that.
>

Well, I totally agree with both of you that this is the superior approach
for architecture and maintainability. Maybe I should think about adding a
bit more setup to my own unit tests.

I think it is not quite right to say that Fraction is the unit. I think
unit tests test atomic behaviors in the code that ideally can only fail one
way; those are the units. But this is just semantics.

So if you are both in agreement I can change to a +1.
Reply | Threaded
Open this post in threaded view
|

Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest

Heinrich Bohne
In reply to this post by Heinrich Bohne
I've created a pull request:
https://github.com/apache/commons-numbers/pull/55

This pull request exterminates most the code duplication between the two
classes. There is still some duplication left, most notably in the
method testDigitLimitConstructor(), but I've found out that the
Fraction(double, double, int, int) constructor (and the corresponding
BigFraction constructor, which seems to have been copy-pasted from
Fraction) doesn't work properly anyway, because it doesn't always
generate the closest possible approximation within the given bounds.

For example, 5/8 = 0.625 is closer to 0.6152 than 3/5 = 0.6, but
apparently, the constructor returns 3/5 when 9 is passed as
maxDenominator, and since the test asserts this, the test is useless.

I saw that there's a class ContinuedFraction, maybe this class can
somehow be used within the constructor instead of duplicating broken
code between two classes, so I didn't bother extracting the above tests.

Also, I created a new branch and renamed the commits to include the
number of the JIRA ticket, which means that the revision numbers are now
different from those in the branch I previously referenced, but the
changes made in the commits are the same, in case anyone is confused.

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]