Merge ~bryce/ubuntu/+source/php-parser:sru-lp1895878-disable-tests into ubuntu/+source/php-parser:ubuntu/focal

[Bryce Harrington]

This disables five test cases that are failing in focal in order to allow a php SRU to finish migration.

Two of the test case failures we encountered in groovy as well, and are known issues upstream. The other three I suspect were caused when other elements of the php stack were updated, however I've not determined a root cause for them. When php7.4 itself transitioned, I don't recall php-parser having build or test issues; however there were some pieces like phpunit, that were transitioning in parallel, and it's possible there were unresolved late-cycle transition issues as a result.

In one of last week's standups we discussed options: a) bypass the testsuite entirely, b) disable just the failing test cases, or c) diagnose the exact problems and identify necessary fixes. For expediency, we preferred (a) or (b), with (b) being felt to be more targeted and less brute-force than (a).

PPA:
 - https://launchpad.net/~bryce/+archive/ubuntu/php-parser-sru-lp1895878-disable-tests

SRU:
 - https://bugs.launchpad.net/ubuntu/+source/php-parser/+bug/1878102
 - https://bugs.launchpad.net/ubuntu/+source/php-parser/+bug/1895878

[Christian Ehrhardt  (paelzer)]

You mention (LP: #1878102) twice in the changelog.
I understand what you mean but I happened to run into issues due to that by some of the tools that run for the automated bug updates and such.
Therefore I'd recommend to re-order the changelog to list the bgu ref only once and the two changes below it.

review: Needs Fixing

[Christian Ehrhardt  (paelzer)]

All the patches miss proper dep3 headers, I can see that we can derive most of the info of the git-format patch that this is - which is mostly fine.
But a few things to add:

1. at least a Bug-Ubuntu: ... should be added to all of them to have a link back from the patch without having to memorize which bug the changelog associates a patch file.

2. debian/patches/disable-broken-test-case-in-EmulativeTest.patch
"Upstream reports this may not be 32-bit compatible."
Is there any link of that we could add as "Origin" ?

[Christian Ehrhardt  (paelzer)]

In general I agree that skipping (b) the affected tests is the best we can do without wasting too much time (c). So thank you for that, but at least in my mind that was meant as "armhf only".

All of the patches should IMHO be done like "if machine.arch() == armhf then skip" or something like it. Do you think it would be possible to do it that way without wasting too much further time on it?

review: Needs Fixing

[Bryce Harrington (bryce)]

Regarding armhf, I am able to reproduce most of these failures on my own amd64 machine, and an earlier test in a ppa showed they fail on other arch's as well. Honestly, all of these php test failures kind of bleed together in my memory at this point, and I can't recall precisely if this was one I definitely narrowed to just armhf, or if perhaps maybe it took longer to run than the others so lost a race against whatever other package caused the breakage. In any case, the tests definitely fail on more than just armhf currently, so conditionalizing on machine arch would not be a complete fix.

As a side note, the original impetuous for this sru was to enable another php7.4 SRU to go through. But that php sru has now gone through (thanks to a CVE bumping it in). So, while the issue still exists here with php-parser, at least the near term pressure is off. Given that, do you think we should hold off until the failures can be more thoroughly diagnosed, or should we press ahead so that next time there's a php sru it doesn't bog down?

In either case, I'll incorporate your other suggested changes before backburnering or proceeding.

[Christian Ehrhardt  (paelzer)]

On Mon, Oct 19, 2020 at 9:47 PM Bryce Harrington <email address hidden>
wrote:

> Regarding armhf, I am able to reproduce most of these failures on my own
> amd64 machine, and an earlier test in a ppa showed they fail on other
> arch's as well. Honestly, all of these php test failures kind of bleed
> together in my memory at this point, and I can't recall precisely if this
> was one I definitely narrowed to just armhf, or if perhaps maybe it took
> longer to run than the others so lost a race against whatever other package
> caused the breakage. In any case, the tests definitely fail on more than
> just armhf currently, so conditionalizing on machine arch would not be a
> complete fix.
>

Ok then, thanks for the explanation!

As a side note, the original impetuous for this sru was to enable another
> php7.4 SRU to go through. But that php sru has now gone through (thanks to
> a CVE bumping it in). So, while the issue still exists here with
> php-parser, at least the near term pressure is off. Given that, do you
> think we should hold off until the failures can be more thoroughly
> diagnosed, or should we press ahead so that next time there's a php sru it
> doesn't bog down?
>

IMHO I'd continue to fix this as intended (neither stall, nor switch to
spend 5 more days).

> In either case, I'll incorporate your other suggested changes before
> backburnering or proceeding.
>

Ok, once those are in I think you are all set then - thanks for the
discussion.

[Bryce Harrington (bryce)]

Thanks, changes applied, tagged and uploaded:

$ git ubuntu tag --upload
$ git push pkg upload/4.2.2-2ubuntu0.1
Enumerating objects: 43, done.
Counting objects: 100% (43/43), done.
Delta compression using up to 12 threads
Compressing objects: 100% (38/38), done.
Writing objects: 100% (38/38), 6.70 KiB | 428.00 KiB/s, done.
Total 38 (delta 25), reused 0 (delta 0)
To ssh://git.launchpad.net/ubuntu/+source/php-parser
 * [new tag] upload/4.2.2-2ubuntu0.1 -> upload/4.2.2-2ubuntu0.1
$ dput ubuntu php-parser_4.2.2-2ubuntu0.1_source.changes
Checking signature on .changes
gpg: /home/bryce/pkg/PhpParser/sru-lp1895878-disable-tests-armhf/php-parser_4.2.2-2ubuntu0.1_source.changes: Valid signature from E603B2578FB8F0FB
Checking signature on .dsc
gpg: /home/bryce/pkg/PhpParser/sru-lp1895878-disable-tests-armhf/php-parser_4.2.2-2ubuntu0.1.dsc: Valid signature from E603B2578FB8F0FB
Uploading to ubuntu (via ftp to upload.ubuntu.com):
  Uploading php-parser_4.2.2-2ubuntu0.1.dsc: done.
  Uploading php-parser_4.2.2-2ubuntu0.1.debian.tar.xz: done.
  Uploading php-parser_4.2.2-2ubuntu0.1_source.buildinfo: done.
  Uploading php-parser_4.2.2-2ubuntu0.1_source.changes: done.
Successfully uploaded packages.

review: Approve

Unmerged commits

c7b0b79... by Bryce Harrington on 2020-10-13

changelog

44a2ca9... by Bryce Harrington on 2020-10-13

 * d/p/skip-EmulativeTest-testErrorAfterEmulation.patch,
   d/p/skip-LexerTest-testError.patch,
   d/p/skip-PrettyPrinterTest.patch: Disable test cases that fail on
   focal, but not groovy.
   (LP: #1878102)

72524b7... by Bryce Harrington on 2020-09-30

changelog

c289125... by Bryce Harrington on 2020-09-30

  * d/p/disable-CodeParsingTest-due-to-integer-syntax.patch:
    Disable another test case that is not yet 32-bit compatible, to fix
    proposed migration blockage on armhf.
    (LP: #1895878)

e3ee89d... by Bryce Harrington on 2020-09-30

  * d/p/disable-broken-test-case-in-EmulativeTest.patch:
    Fix FTBFS by disabling a test case upstream suggests may not
    yet be adequately checked for all architectures.
    (LP: #1878102)

diff --git a/debian/changelog b/debian/changelog
index 966163c..ded4105 100644
--- a/debian/changelog
+++ b/debian/changelog
@@ -1,3 +1,21 @@
+php-parser (4.2.2-2ubuntu0.1) focal; urgency=medium
+
+  * d/p/disable-broken-test-case-in-EmulativeTest.patch:
+    Fix FTBFS by disabling a test case upstream suggests may not
+    yet be adequately checked for all architectures.
+    (LP: #1878102)
+  * d/p/disable-CodeParsingTest-due-to-integer-syntax.patch:
+    Disable another test case that is not yet 32-bit compatible, to fix
+    proposed migration blockage on armhf.
+    (LP: #1895878)
+  * d/p/skip-EmulativeTest-testErrorAfterEmulation.patch,
+    d/p/skip-LexerTest-testError.patch, d/p/skip-PrettyPrinterTest.patch:
+    Disable additional parser/lexer test cases that fail on focal, but not
+    groovy.
+    (LP: #1878102)
+
+ -- Bryce Harrington <[e-mail]>  Wed, 30 Sep 2020 22:14:10 +0000
+
 php-parser (4.2.2-2) unstable; urgency=medium
 
   * Upload to unstable in sync with phpdox
diff --git a/debian/patches/disable-CodeParsingTest-due-to-integer-syntax.patch b/debian/patches/disable-CodeParsingTest-due-to-integer-syntax.patch
new file mode 100644
index 0000000..9cac701
--- /dev/null
+++ b/debian/patches/disable-CodeParsingTest-due-to-integer-syntax.patch
@@ -0,0 +1,65 @@
+From a354b3bc4fcf1bfe22d84741ebd1d945c4fa50df Mon Sep 17 00:00:00 2001
+From: Bryce Harrington <[e-mail]>
+Date: Thu, 14 May 2020 23:11:18 +0000
+Subject: [PATCH] Disable CodeParsingTest due to integer syntax inconsistency
+ on armhf
+
+Workaround testsuite failure when run on armhf by disabling the test
+case.  The specific failing test case is from
+PhpParser\CodeParsingTest::testParse with the
+scalar/numberSeparators.test#0 data set:
+
+      2: Stmt_Expression(
+ -        expr: Scalar_LNumber(
+ -            value: 3405705229
+ +        expr: Scalar_DNumber(
+ +            value: 0
+          )
+
+Signed-off-by: Bryce Harrington <[e-mail]>
+---
+ test/PhpParser/CodeParsingTest.php | 28 ----------------------------
+ 1 file changed, 28 deletions(-)
+
+diff --git a/test/PhpParser/CodeParsingTest.php b/test/PhpParser/CodeParsingTest.php
+index 24e93dd..dba8f1e 100644
+--- a/test/PhpParser/CodeParsingTest.php
++++ b/test/PhpParser/CodeParsingTest.php
+@@ -7,34 +7,6 @@ use PhpParser\Node\Stmt;
+ 
+ class CodeParsingTest extends CodeTestAbstract
+ {
+-    /**
+-     * @dataProvider provideTestParse
+-     */
+-    public function testParse($name, $code, $expected, $modeLine) {
+-        if (null !== $modeLine) {
+-            $modes = array_fill_keys(explode(',', $modeLine), true);
+-        } else {
+-            $modes = [];
+-        }
+-
+-        list($parser5, $parser7) = $this->createParsers($modes);
+-        list($stmts5, $output5) = $this->getParseOutput($parser5, $code, $modes);
+-        list($stmts7, $output7) = $this->getParseOutput($parser7, $code, $modes);
+-
+-        if (isset($modes['php5'])) {
+-            $this->assertSame($expected, $output5, $name);
+-            $this->assertNotSame($expected, $output7, $name);
+-        } elseif (isset($modes['php7'])) {
+-            $this->assertNotSame($expected, $output5, $name);
+-            $this->assertSame($expected, $output7, $name);
+-        } else {
+-            $this->assertSame($expected, $output5, $name);
+-            $this->assertSame($expected, $output7, $name);
+-        }
+-
+-        $this->checkAttributes($stmts5);
+-        $this->checkAttributes($stmts7);
+-    }
+ 
+     public function createParsers(array $modes) {
+         $lexer = new Lexer\Emulative(['usedAttributes' => [
+-- 
+2.25.1
+
diff --git a/debian/patches/disable-broken-test-case-in-EmulativeTest.patch b/debian/patches/disable-broken-test-case-in-EmulativeTest.patch
new file mode 100644
index 0000000..61fc1a1
--- /dev/null
+++ b/debian/patches/disable-broken-test-case-in-EmulativeTest.patch
@@ -0,0 +1,38 @@
+From 6bf75f03c7e498e7339e21c6a1ba3cf5a0ccf336 Mon Sep 17 00:00:00 2001
+From: Bryce Harrington <[e-mail]>
+Date: Wed, 30 Sep 2020 21:42:18 +0000
+Subject: [PATCH] test: Disable broken test case in EmulativeTest
+
+Upstream reports this may not be 32-bit compatible.
+---
+ test/PhpParser/Lexer/EmulativeTest.php | 14 --------------
+ 1 file changed, 14 deletions(-)
+
+diff --git a/test/PhpParser/Lexer/EmulativeTest.php b/test/PhpParser/Lexer/EmulativeTest.php
+index a53c379..b67e659 100644
+--- a/test/PhpParser/Lexer/EmulativeTest.php
++++ b/test/PhpParser/Lexer/EmulativeTest.php
+@@ -70,20 +70,6 @@ class EmulativeTest extends LexerTest
+         ];
+     }
+ 
+-    /**
+-     * @dataProvider provideTestLexNewFeatures
+-     */
+-    public function testLexNewFeatures($code, array $expectedTokens) {
+-        $lexer = $this->getLexer();
+-        $lexer->startLexing('<?php ' . $code);
+-
+-        $tokens = [];
+-        while (0 !== $token = $lexer->getNextToken($text)) {
+-            $tokens[] = [$token, $text];
+-        }
+-        $this->assertSame($expectedTokens, $tokens);
+-    }
+-
+     /**
+      * @dataProvider provideTestLexNewFeatures
+      */
+-- 
+2.25.1
+
diff --git a/debian/patches/series b/debian/patches/series
index 57b1eeb..589922b 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -1,3 +1,10 @@
 0001-Allow-require-to-search-in-the-path.patch
 0002-Adapt-shebang-for-PHP-script.patch
 0003-Compatibility-with-recent-PHPUnit.patch
+
+# Ubuntu
+disable-broken-test-case-in-EmulativeTest.patch
+disable-CodeParsingTest-due-to-integer-syntax.patch
+skip-EmulativeTest-testErrorAfterEmulation.patch
+skip-LexerTest-testError.patch
+skip-PrettyPrinterTest.patch
diff --git a/debian/patches/skip-EmulativeTest-testErrorAfterEmulation.patch b/debian/patches/skip-EmulativeTest-testErrorAfterEmulation.patch
new file mode 100644
index 0000000..991fe51
--- /dev/null
+++ b/debian/patches/skip-EmulativeTest-testErrorAfterEmulation.patch
@@ -0,0 +1,26 @@
+From 1ceca6f34d2e736fe6782e91358055b158a8cfe5 Mon Sep 17 00:00:00 2001
+From: Bryce Harrington <[e-mail]>
+Date: Mon, 5 Oct 2020 13:19:30 -0700
+Subject: [PATCH] Skip EmulativeTest::testErrorAfterEmulation
+
+---
+ test/PhpParser/Lexer/EmulativeTest.php | 17 +++--------------
+ 1 file changed, 3 insertions(+), 14 deletions(-)
+
+diff --git a/test/PhpParser/Lexer/EmulativeTest.php b/test/PhpParser/Lexer/EmulativeTest.php
+index a53c379..7b9bf28 100644
+--- a/test/PhpParser/Lexer/EmulativeTest.php
++++ b/test/PhpParser/Lexer/EmulativeTest.php
+@@ -102,6 +88,9 @@ class EmulativeTest extends LexerTest
+      * @dataProvider provideTestLexNewFeatures
+      */
+     public function testErrorAfterEmulation($code) {
++        // Skip testErrorAfterEmulation() (LP: #1878102)
++	return;
++
+         $errorHandler = new ErrorHandler\Collecting;
+         $lexer = $this->getLexer();
+         $lexer->startLexing('<?php ' . $code . "\0", $errorHandler);
+-- 
+2.25.1
+
diff --git a/debian/patches/skip-LexerTest-testError.patch b/debian/patches/skip-LexerTest-testError.patch
new file mode 100644
index 0000000..8144cdb
--- /dev/null
+++ b/debian/patches/skip-LexerTest-testError.patch
@@ -0,0 +1,26 @@
+From 90b6aa2e2fbef8e1a22e99b50a7ec17317d96da9 Mon Sep 17 00:00:00 2001
+From: Bryce Harrington <[e-mail]>
+Date: Mon, 5 Oct 2020 13:27:38 -0700
+Subject: [PATCH] Disable LexerTest::testError
+
+---
+ test/PhpParser/LexerTest.php | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/test/PhpParser/LexerTest.php b/test/PhpParser/LexerTest.php
+index f24c64d..19036ef 100644
+--- a/test/PhpParser/LexerTest.php
++++ b/test/PhpParser/LexerTest.php
+@@ -15,6 +15,9 @@ class LexerTest extends \PHPUnit\Framework\TestCase
+      * @dataProvider provideTestError
+      */
+     public function testError($code, $messages) {
++        // Skip EmulativeTest::testError (LP: #1878102)
++	return;
++
+         if (defined('HHVM_VERSION')) {
+             $this->markTestSkipped('HHVM does not throw warnings from token_get_all()');
+         }
+-- 
+2.25.1
+
diff --git a/debian/patches/skip-PrettyPrinterTest.patch b/debian/patches/skip-PrettyPrinterTest.patch
new file mode 100644
index 0000000..934ee23
--- /dev/null
+++ b/debian/patches/skip-PrettyPrinterTest.patch
@@ -0,0 +1,26 @@
+From 2831a91e247d9c05f6b2b67d3d66baee8854780c Mon Sep 17 00:00:00 2001
+From: Bryce Harrington <[e-mail]>
+Date: Mon, 5 Oct 2020 13:17:32 -0700
+Subject: [PATCH] Skip PrettyPrinterTest
+
+---
+ test/PhpParser/PrettyPrinterTest.php | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/test/PhpParser/PrettyPrinterTest.php b/test/PhpParser/PrettyPrinterTest.php
+index e5b8a1a..c266234 100644
+--- a/test/PhpParser/PrettyPrinterTest.php
++++ b/test/PhpParser/PrettyPrinterTest.php
+@@ -283,6 +283,9 @@ CODE
+ 
+         $printer = new PrettyPrinter\Standard();
+ 
++	// Skip test
++	return;
++
+         try {
+             $oldStmts = $parser->parse($code);
+         } catch (Error $e) {
+-- 
+2.25.1
+