Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
> > > 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.
Re: Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
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 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 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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
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 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 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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail:
Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
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 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 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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
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 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 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: dev-unsubscr...@commons.apache.org > >> For additional commands, e-mail: dev-h...@commons.apache.org > >> > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
Re: Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
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 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: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [numbers-fraction] Code duplication between FractionTest and BigFractionTest
> On 20 Jun 2019, at 00:54, Heinrich Bohne 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: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
