From d27b6a672a030fa949ccbaedda25b14b335d906d Mon Sep 17 00:00:00 2001 From: oleibman <10341515+oleibman@users.noreply.github.com> Date: Sat, 1 Oct 2022 07:05:54 -0700 Subject: [PATCH] Cleanup 3 LookupRef Tests (#3097) Scrutinizer had previously suggested annotations for 3 LookupRef tests, but it no longer accepts its own annotation for those cases. This PR cleans them up. ColumnsTest and RowsTest are extremely straightforward. IndexTest is a bit more complicated, but only because, unlike the other two, it had no test which executed in the context of a spreadsheet. And, when I added those, I discovered a couple of bugs. INDEX always requires at least 2 parameters (row# is always required), but its entry in the function table specified 1-4 parameters, now changed to 2-4. And, omitting col# is not handled the same way as specifying 0 for col#, though the code had treated them identically. (The same would have been true for row# but, because it is now required, ...) --- .../Calculation/Calculation.php | 2 +- .../Calculation/LookupRef/Matrix.php | 6 +- .../Functions/LookupRef/ColumnsTest.php | 5 +- .../LookupRef/IndexOnSpreadsheetTest.php | 39 +++ .../Functions/LookupRef/IndexTest.php | 15 +- .../Functions/LookupRef/RowsTest.php | 5 +- .../LookupRef/INDEXonSpreadsheet.php | 234 ++++++++++++++++++ 7 files changed, 297 insertions(+), 9 deletions(-) create mode 100644 tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexOnSpreadsheetTest.php create mode 100644 tests/data/Calculation/LookupRef/INDEXonSpreadsheet.php diff --git a/src/PhpSpreadsheet/Calculation/Calculation.php b/src/PhpSpreadsheet/Calculation/Calculation.php index 5725795e..066ed0ee 100644 --- a/src/PhpSpreadsheet/Calculation/Calculation.php +++ b/src/PhpSpreadsheet/Calculation/Calculation.php @@ -1450,7 +1450,7 @@ class Calculation 'INDEX' => [ 'category' => Category::CATEGORY_LOOKUP_AND_REFERENCE, 'functionCall' => [LookupRef\Matrix::class, 'index'], - 'argumentCount' => '1-4', + 'argumentCount' => '2-4', ], 'INDIRECT' => [ 'category' => Category::CATEGORY_LOOKUP_AND_REFERENCE, diff --git a/src/PhpSpreadsheet/Calculation/LookupRef/Matrix.php b/src/PhpSpreadsheet/Calculation/LookupRef/Matrix.php index a447e203..d7d15d4e 100644 --- a/src/PhpSpreadsheet/Calculation/LookupRef/Matrix.php +++ b/src/PhpSpreadsheet/Calculation/LookupRef/Matrix.php @@ -76,13 +76,14 @@ class Matrix * If an array of values is passed as the $rowNum and/or $columnNum arguments, then the returned result * will also be an array with the same dimensions */ - public static function index($matrix, $rowNum = 0, $columnNum = 0) + public static function index($matrix, $rowNum = 0, $columnNum = null) { if (is_array($rowNum) || is_array($columnNum)) { return self::evaluateArrayArgumentsSubsetFrom([self::class, __FUNCTION__], 1, $matrix, $rowNum, $columnNum); } $rowNum = $rowNum ?? 0; + $originalColumnNum = $columnNum; $columnNum = $columnNum ?? 0; try { @@ -102,6 +103,9 @@ class Matrix if ($columnNum > count($columnKeys)) { return ExcelError::REF(); } + if ($originalColumnNum === null && 1 < count($columnKeys)) { + return ExcelError::REF(); + } if ($columnNum === 0) { return self::extractRowValue($matrix, $rowKeys, $rowNum); diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/ColumnsTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/ColumnsTest.php index e8790cbf..67135af0 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/ColumnsTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/ColumnsTest.php @@ -12,10 +12,11 @@ class ColumnsTest extends TestCase * @dataProvider providerCOLUMNS * * @param mixed $expectedResult + * @param null|array|string $arg */ - public function testCOLUMNS($expectedResult, ...$args): void + public function testCOLUMNS($expectedResult, $arg): void { - $result = LookupRef::COLUMNS(/** @scrutinizer ignore-type */ ...$args); + $result = LookupRef::COLUMNS($arg); self::assertEquals($expectedResult, $result); } diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexOnSpreadsheetTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexOnSpreadsheetTest.php new file mode 100644 index 00000000..de3832b9 --- /dev/null +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexOnSpreadsheetTest.php @@ -0,0 +1,39 @@ +mightHaveException($expectedResult); + $sheet = $this->getSheet(); + $sheet->fromArray($matrix); + $maxRow = $sheet->getHighestRow(); + $maxColumn = $sheet->getHighestColumn(); + $formulaArray = "A1:$maxColumn$maxRow"; + if ($rowNum === null) { + $formula = "=INDEX($formulaArray)"; + } elseif ($colNum === null) { + $formula = "=INDEX($formulaArray, $rowNum)"; + } else { + $formula = "=INDEX($formulaArray, $rowNum, $colNum)"; + } + $sheet->getCell('ZZ98')->setValue('x'); + $sheet->getCell('ZZ99')->setValue($formula); + $result = $sheet->getCell('ZZ99')->getCalculatedValue(); + self::assertEquals($expectedResult, $result); + } + + public function providerINDEXonSpreadsheet(): array + { + return require 'tests/data/Calculation/LookupRef/INDEXonSpreadsheet.php'; + } +} diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexTest.php index fa3f4848..3fe5d041 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/IndexTest.php @@ -3,7 +3,7 @@ namespace PhpOffice\PhpSpreadsheetTests\Calculation\Functions\LookupRef; use PhpOffice\PhpSpreadsheet\Calculation\Calculation; -use PhpOffice\PhpSpreadsheet\Calculation\LookupRef; +use PhpOffice\PhpSpreadsheet\Calculation\LookupRef\Matrix; use PHPUnit\Framework\TestCase; class IndexTest extends TestCase @@ -12,10 +12,19 @@ class IndexTest extends TestCase * @dataProvider providerINDEX * * @param mixed $expectedResult + * @param mixed $matrix + * @param mixed $rowNum + * @param mixed $colNum */ - public function testINDEX($expectedResult, ...$args): void + public function testINDEX($expectedResult, $matrix, $rowNum = null, $colNum = null): void { - $result = LookupRef::INDEX(/** @scrutinizer ignore-type */ ...$args); + if ($rowNum === null) { + $result = Matrix::index($matrix); + } elseif ($colNum === null) { + $result = Matrix::index($matrix, $rowNum); + } else { + $result = Matrix::index($matrix, $rowNum, $colNum); + } self::assertEquals($expectedResult, $result); } diff --git a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/RowsTest.php b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/RowsTest.php index fd9902f4..33c9dc45 100644 --- a/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/RowsTest.php +++ b/tests/PhpSpreadsheetTests/Calculation/Functions/LookupRef/RowsTest.php @@ -12,10 +12,11 @@ class RowsTest extends TestCase * @dataProvider providerROWS * * @param mixed $expectedResult + * @param null|array|string $arg */ - public function testROWS($expectedResult, ...$args): void + public function testROWS($expectedResult, $arg): void { - $result = LookupRef::ROWS(/** @scrutinizer ignore-type */ ...$args); + $result = LookupRef::ROWS($arg); self::assertEquals($expectedResult, $result); } diff --git a/tests/data/Calculation/LookupRef/INDEXonSpreadsheet.php b/tests/data/Calculation/LookupRef/INDEXonSpreadsheet.php new file mode 100644 index 00000000..eff13722 --- /dev/null +++ b/tests/data/Calculation/LookupRef/INDEXonSpreadsheet.php @@ -0,0 +1,234 @@ + [ + 1, // Expected + [ + [1], + ], + 0, + ], + 'Row Number omitted' => [ + 'exception', // Expected + [ + [1], + ], + ], + 'Negative Row' => [ + '#VALUE!', // Expected + [ + [1], + [2], + ], + -1, + ], + 'Row > matrix rows' => [ + '#REF!', // Expected + [ + [1], + [2], + ], + 10, + ], + 'Row is not a number' => [ + '#NAME?', // Expected + [ + [1], + [2], + ], + 'NaN', + ], + 'Row is reference to non-number' => [ + '#VALUE!', // Expected + [ + [1], + [2], + ], + 'ZZ98', + ], + 'Row is quoted non-numeric result' => [ + '#VALUE!', // Expected + [ + [1], + [2], + ], + '"string"', + ], + 'Row is Error' => [ + '#N/A', // Expected + [ + [1], + [2], + ], + '#N/A', + ], + 'Return row 2 only one column' => [ + 'xyz', // Expected + [ + ['abc'], + ['xyz'], + ], + 2, + ], + 'Return row 1 col 2' => [ + 'def', // Expected + [ + ['abc', 'def'], + ['xyz', 'tuv'], + ], + 1, + 2, + ], + 'Column number omitted from 2-column matrix' => [ + '#REF!', // Expected + [ + ['abc', 'def'], + ['xyz', 'tuv'], + ], + 1, + ], + 'Column number omitted from 1-column matrix' => [ + 'xyz', // Expected + [ + ['abc'], + ['xyz'], + ], + 2, + ], + 'Return row 2 from larger matrix (Phpspreadsheet flattens expected [2,4] to single value)' => [ + 2, // Expected + // Input + [ + [1, 3], + [2, 4], + ], + 2, + 0, + ], + 'Negative Column' => [ + '#VALUE!', // Expected + [ + [1, 3], + [2, 4], + ], + 0, + -1, + ], + 'Column > matrix columns' => [ + '#REF!', // Expected + [ + [1, 3], + [2, 4], + ], + 2, + 10, + ], + 'Column is not a number' => [ + '#NAME?', // Expected + [ + [1], + [2], + ], + 1, + 'NaN', + ], + 'Column is reference to non-number' => [ + '#VALUE!', // Expected + [ + [1], + [2], + ], + 1, + 'ZZ98', + ], + 'Column is quoted non-number' => [ + '#VALUE!', // Expected + [ + [1], + [2], + ], + 1, + '"string"', + ], + 'Column is Error' => [ + '#N/A', // Expected + [ + [1], + [2], + ], + 1, + '#N/A', + ], + 'Row 2 Column 2' => [ + 4, // Expected + [ + [1, 3], + [2, 4], + ], + 2, + 2, + ], + 'Row 2 Column 2 Alphabetic' => [ + 'Pears', + [ + ['Apples', 'Lemons'], + ['Bananas', 'Pears'], + ], + 2, + 2, + ], + 'Row 2 Column 1 Alphabetic' => [ + 'Bananas', + [ + ['Apples', 'Lemons'], + ['Bananas', 'Pears'], + ], + 2, + 1, + ], + 'Row 2 Column 0 (PhpSpreadsheet flattens result)' => [ + 'Bananas', + [ + ['Apples', 'Lemons'], + ['Bananas', 'Pears'], + ], + 2, + 0, + ], + 'Row 5 column 2' => [ + 3, + [ + [4, 6], + [5, 3], + [6, 9], + [7, 5], + [8, 3], + ], + 5, + 2, + ], + 'Row 5 column 0 (flattened)' => [ + 8, + [ + [4, 6], + [5, 3], + [6, 9], + [7, 5], + [8, 3], + ], + 5, + 0, + ], + 'Row 0 column 2 (flattened)' => [ + 6, + [ + [4, 6], + [5, 3], + [6, 9], + [7, 5], + [8, 3], + ], + 0, + 2, + ], +];