From abc1d3db70c51d59ad60ae493bd58e7985392598 Mon Sep 17 00:00:00 2001
From: oleibman <10341515+oleibman@users.noreply.github.com>
Date: Sat, 23 Jul 2022 07:46:32 -0700
Subject: [PATCH] Big Memory Leak in One Test (#2958)

* 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.
---
 .../Reader/Xlsx/Issue2301Test.php             |   2 +
 .../ConditionalFormatting/CellMatcherTest.php | 105 +++++++-----------
 2 files changed, 44 insertions(+), 63 deletions(-)

diff --git a/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2301Test.php b/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2301Test.php
index 4484b9e0..7a5829f7 100644
--- a/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2301Test.php
+++ b/tests/PhpSpreadsheetTests/Reader/Xlsx/Issue2301Test.php
@@ -24,5 +24,7 @@ class Issue2301Test extends \PHPUnit\Framework\TestCase
         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);
     }
 }
diff --git a/tests/PhpSpreadsheetTests/Style/ConditionalFormatting/CellMatcherTest.php b/tests/PhpSpreadsheetTests/Style/ConditionalFormatting/CellMatcherTest.php
index 3c96403b..2c6d0da8 100644
--- a/tests/PhpSpreadsheetTests/Style/ConditionalFormatting/CellMatcherTest.php
+++ b/tests/PhpSpreadsheetTests/Style/ConditionalFormatting/CellMatcherTest.php
@@ -10,15 +10,24 @@ use PHPUnit\Framework\TestCase;
 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;
+        }
     }
 
     /**
@@ -26,16 +35,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -82,16 +88,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -128,16 +131,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -167,16 +167,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -216,16 +213,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -329,16 +323,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -365,16 +356,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -400,16 +388,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -447,16 +432,13 @@ class CellMatcherTest extends TestCase
      */
     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);
@@ -486,16 +468,13 @@ class CellMatcherTest extends TestCase
      */
     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);