Skip to content

Commit

Permalink
Big Memory Leak in One Test (#2958)
Browse files Browse the repository at this point in the history 
* Big Memory Leak in One Test

Many tests leak a little by not issuing disconnectWorksheets at the end. CellMatcherTest leaks a lot by opening a spreadsheet many times. Phpunit reported a high watermark of 390MB; fixing CellMatcherTest brings that figure down to 242MB. I have also changed it to use a formal assertion in many cases where markTestSkipped was (IMO inappropriately) used.

* Another Leak

Issue2301Test lacks a disconnect. Adding one reduces HWM from 242MB to 224.
  • Loading branch information
@oleibman
oleibman authored Jul 23, 2022
1 parent 0ee1330 commit abc1d3d
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 63 deletions.
2 changes: 2 additions & 0 deletions tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2301Test.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,5 +24,7 @@ public static function testReadRichText(): void
self::assertSame('Arial CE', $font->getName());
self::assertSame(9.0, $font->getSize());
self::assertSame('protected', $sheet->getCell('BT10')->getStyle()->getProtection()->getHidden());
$spreadsheet->disconnectWorksheets();
unset($spreadsheet);
}
}
105 changes: 42 additions & 63 deletions tests/PhpSpreadsheetTests/Style/ConditionalFormatting/CellMatcherTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,32 +10,38 @@
class CellMatcherTest extends TestCase
{
/**
* @var Spreadsheet
* @var ?Spreadsheet
*/
protected $spreadsheet;

protected function setUp(): void
protected function loadSpreadsheet(): Spreadsheet
{
$filename = 'tests/data/Style/ConditionalFormatting/CellMatcher.xlsx';
$reader = IOFactory::createReader('Xlsx');
$this->spreadsheet = $reader->load($filename);

return $reader->load($filename);
}

protected function tearDown(): void
{
if ($this->spreadsheet !== null) {
$this->spreadsheet->disconnectWorksheets();
$this->spreadsheet = null;
}
}

/**
* @dataProvider basicCellIsComparisonDataProvider
*/
public function testBasicCellIsComparison(string $sheetname, string $cellAddress, array $expectedMatches): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "{$cellAddress} is not in a Conditional Format range");
$cfStyles = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down Expand Up @@ -82,16 +88,13 @@ public function basicCellIsComparisonDataProvider(): array
*/
public function testRangeCellIsComparison(string $sheetname, string $cellAddress, bool $expectedMatch): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyle = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down Expand Up @@ -128,16 +131,13 @@ public function rangeCellIsComparisonDataProvider(): array
*/
public function testCellIsMultipleExpression(string $sheetname, string $cellAddress, array $expectedMatches): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyles = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down Expand Up @@ -167,16 +167,13 @@ public function cellIsExpressionMultipleDataProvider(): array
*/
public function testCellIsExpression(string $sheetname, string $cellAddress, bool $expectedMatch): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyle = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down Expand Up @@ -216,16 +213,13 @@ public function cellIsExpressionDataProvider(): array
*/
public function testTextExpressions(string $sheetname, string $cellAddress, bool $expectedMatch): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyle = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down Expand Up @@ -329,16 +323,13 @@ public function textExpressionsDataProvider(): array
*/
public function testBlankExpressions(string $sheetname, string $cellAddress, array $expectedMatches): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyles = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand All @@ -365,16 +356,13 @@ public function blanksDataProvider(): array
*/
public function testErrorExpressions(string $sheetname, string $cellAddress, array $expectedMatches): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyles = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand All @@ -400,16 +388,13 @@ public function errorDataProvider(): array
*/
public function testDateOccurringExpressions(string $sheetname, string $cellAddress, bool $expectedMatch): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyle = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down Expand Up @@ -447,16 +432,13 @@ public function dateOccurringDataProvider(): array
*/
public function testDuplicatesExpressions(string $sheetname, string $cellAddress, array $expectedMatches): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::AssertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyles = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down Expand Up @@ -486,16 +468,13 @@ public function duplicatesDataProvider(): array
*/
public function testCrossWorksheetExpressions(string $sheetname, string $cellAddress, bool $expectedMatch): void
{
$this->spreadsheet = $this->loadSpreadsheet();
$worksheet = $this->spreadsheet->getSheetByName($sheetname);
if ($worksheet === null) {
self::markTestSkipped("{$sheetname} not found in test workbook");
}
self::assertNotNull($worksheet, "$sheetname not found in test workbook");
$cell = $worksheet->getCell($cellAddress);

$cfRange = $worksheet->getConditionalRange($cell->getCoordinate());
if ($cfRange === null) {
self::markTestSkipped("{$cellAddress} is not in a Conditional Format range");
}
self::assertNotNull($cfRange, "$cellAddress is not in a Conditional Format range");
$cfStyle = $worksheet->getConditionalStyles($cell->getCoordinate());

$matcher = new CellMatcher($cell, $cfRange);
Expand Down

0 comments on commit abc1d3d

Please sign in to comment.