kevingurney commented on code in PR #39586: URL: https://github.com/apache/arrow/pull/39586#discussion_r1450882455
########## matlab/test/arrow/type/tDate64Type.m: ########## @@ -15,7 +15,9 @@ % implied. See the License for the specific language governing % permissions and limitations under the License. -classdef tDate64Type < hFixedWidthType +% Modified test class for arrow.type.Date64Type and arrow.date64 + +classdef tDate64Type < tDataTypeTest Review Comment: ```suggestion classdef tDate64Type < tDateTypeTest ``` ########## matlab/test/arrow/type/tDate32Type.m: ########## @@ -15,7 +15,9 @@ % implied. See the License for the specific language governing % permissions and limitations under the License. -classdef tDate32Type < hFixedWidthType +% Modified test class for arrow.type.Date32Type and arrow.date32 + +classdef tDate32Type < tDataTypeTest Review Comment: ```suggestion classdef tDate32Type < tDateTypeTest ``` ########## matlab/test/arrow/type/tDate32Type.m: ########## @@ -15,7 +15,9 @@ % implied. See the License for the specific language governing % permissions and limitations under the License. -classdef tDate32Type < hFixedWidthType +% Modified test class for arrow.type.Date32Type and arrow.date32 Review Comment: This line can be removed, since there is already a comment at the top of the file describing the class. ########## matlab/test/arrow/type/tDateTypeTest.m: ########## @@ -0,0 +1,80 @@ +% Shared superclass for DateType-related tests for Date32Type and Date64Type +classdef tDateType < matlab.unittest.TestCase Review Comment: Missing the Apache license header in this file. ########## matlab/test/arrow/type/tDateTypeTest.m: ########## @@ -0,0 +1,80 @@ +% Shared superclass for DateType-related tests for Date32Type and Date64Type Review Comment: ```suggestion % Shared superclass for DateType-related tests ``` ########## matlab/test/arrow/type/tDate64Type.m: ########## @@ -15,7 +15,9 @@ % implied. See the License for the specific language governing % permissions and limitations under the License. -classdef tDate64Type < hFixedWidthType +% Modified test class for arrow.type.Date64Type and arrow.date64 Review Comment: This line can be removed, since there is already a comment at the top of the file describing the class. ########## matlab/test/arrow/type/tDateTypeTest.m: ########## @@ -0,0 +1,80 @@ +% Shared superclass for DateType-related tests for Date32Type and Date64Type +classdef tDateType < matlab.unittest.TestCase + + properties + ConstructionFcn + ArrowType + TypeID + BitWidth + ClassName + end + + methods (Test) + function TestClass(testCase) + % Verify ArrowType is an object of the expected class type. + name = string(class(testCase.ArrowType)); + testCase.verifyEqual(name, testCase.ClassName); + end + + function DefaultDateUnit(testCase) + type = testCase.ConstructionFcn(); + actualUnit = type.DateUnit; + expectedUnit = testCase.getDefaultDateUnit(); + testCase.verifyEqual(actualUnit, expectedUnit); + end + + function DateUnitNoSetter(testCase) + % Verify that an error is thrown when trying to set the value + % of the DateUnit property. + type = testCase.ConstructionFcn(); + testCase.verifyError(@() setfield(type, "DateUnit", "Millisecond"), "MATLAB:class:SetProhibited"); + end + + function InvalidProxy(testCase) + % Verify that an error is thrown when a Proxy of an unexpected + % type is passed to the DateType constructor. + array = arrow.array([1, 2, 3]); + proxy = array.Proxy; + testCase.verifyError(@() testCase.ConstructionFcn(proxy), "arrow:proxy:ProxyNameMismatch"); + end + + function IsEqualTrue(testCase) + % Verifies isequal method of DateType returns true if + % conditions are met: + % + % 1. All input arguments have a class type DateType + % 2. All inputs have the same size + + % Scalar DateType arrays + dateType1 = testCase.ConstructionFcn(); + dateType2 = testCase.ConstructionFcn(); + testCase.verifyTrue(isequal(dateType1, dateType2)); + + % Non-scalar DateType arrays + typeArray1 = [dateType1 dateType1]; + typeArray2 = [dateType2 dateType2]; + testCase.verifyTrue(isequal(typeArray1, typeArray2)); + end + + function IsEqualFalse(testCase) + % Verifies the isequal method of DateType returns false when expected. + % Pass a different arrow.type.Type subclass to isequal + dateType = testCase.ConstructionFcn(); + int32Type = arrow.int32(); + testCase.verifyFalse(isequal(dateType, int32Type)); + testCase.verifyFalse(isequal([dateType dateType], [int32Type int32Type])); + + % DateType arrays have different sizes + typeArray1 = [dateType dateType]; + typeArray2 = [dateType dateType]'; + testCase.verifyFalse(isequal(typeArray1, typeArray2)); + end + end + + methods (Access = private) + function defaultUnit = retrieveDefaultDateUnit(~) Review Comment: There are two different names here for the method for retrieving the default date unit. One is `retrieveDefaultDateUnit` and the other is `getDefaultDateUnit`. If the intent is to reuse the inherited `retrieveDefaultDateUnit` method in each subclass, then I would recommend defining the `retrieveDefaultDateUnit` method as abstract in the `tDateTypeTest` superclass and then providing specialized implementations for the method in each `DateType` test subclass. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
