Les tests unitaires plantent #4417

Erreur sur le test unitaire avec tar sur le cas empty chez moi en PHP 7.4

~/Sites/localhost/fraich40/plugins-dist/archiviste/ [master*] vendor/bin/phpunit                                                                                                                                              (master|✚1)
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.21
Configuration: /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/phpunit.xml.dist
Warning:       No code coverage driver available

.E..................                                              20 / 20 (100%)

Time: 00:00.038, Memory: 6.00 MB

There was 1 error:

1) Spip\Archiver\Tests\SpipArchiverTest::testInformer with data set "empty-tar" (0, array(array(''), array()), '/Users/cedric/Sites/localhost...ty.tar', 'tar')
RuntimeException: Directory name must not be empty.

/Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/src/TarArchive.php:44
/Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/src/SpipArchiver.php:37
/Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/tests/SpipArchiverTest.php:163

ERRORS!
Tests: 20, Assertions: 49, Errors: 1.

En debugant je vois que si un fichier n'existe pas $this->tar n'est pas correctement initialisé dans le open()

Le patch suivant corrige ce cas en verifiant que l'instantiation eu lieu correctement :

diff --git a/src/TarArchive.php b/src/TarArchive.php
index 693c2ae..602d568 100644
--- a/src/TarArchive.php
+++ b/src/TarArchive.php
@@ -26,6 +26,10 @@ class TarArchive implements ArchiveInterface
 			2
 		);

+		if ('' === $this->tar->getFilename()) {
+			return 0;
+		}
+
 		return 1;
 	}

@@ -38,7 +42,6 @@ class TarArchive implements ArchiveInterface
 		if ('' === $this->tar->getPathname()) {
 			return $files;
 		}
-
 		$root_dir = dirname($this->tar->getPathname());
 		$source = new NoDotFilterIterator(
 			new \RecursiveIteratorIterator(

Et du coup on voit que le test emballer sur le tar ne marche pas (c'était visiblement masqué par le fait qu'on avait pas d'erreur sur un fichier vide avant...)

~/Sites/localhost/fraich40/plugins-dist/archiviste/ [master*] vendor/bin/phpunit                                                                                                                                              (master|✚1)
PHPUnit 9.5.13 by Sebastian Bergmann and contributors.

Runtime:       PHP 7.4.21
Configuration: /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/phpunit.xml.dist
Warning:       No code coverage driver available

.F......F...........                                              20 / 20 (100%)

Time: 00:00.030, Memory: 6.00 MB

There were 2 failures:

1) Spip\Archiver\Tests\SpipArchiverTest::testInformer with data set "empty-tar" (0, array(array(''), array()), '/Users/cedric/Sites/localhost...ty.tar', 'tar')
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
     'proprietes' => Array (
-        'racine' => ''
     )
     'fichiers' => Array ()
 )

/Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/tests/SpipArchiverTest.php:166

2) Spip\Archiver\Tests\SpipArchiverTest::testEmballer with data set "tar" (true, 0, '/Users/cedric/Sites/localhost...er.tar', '', array('/Users/cedric/Sites/localhost...st.txt'), '/Users/cedric/Sites/localhost...ectory')
compress ok
Failed asserting that false matches expected true.

/Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/tests/SpipArchiverTest.php:283

FAILURES!
Tests: 20, Assertions: 48, Failures: 2.

Erreur sur le test unitaire avec tar sur le cas empty chez moi en PHP 7.4 ``` ~/Sites/localhost/fraich40/plugins-dist/archiviste/ [master*] vendor/bin/phpunit (master|✚1) PHPUnit 9.5.13 by Sebastian Bergmann and contributors. Runtime: PHP 7.4.21 Configuration: /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/phpunit.xml.dist Warning: No code coverage driver available .E.................. 20 / 20 (100%) Time: 00:00.038, Memory: 6.00 MB There was 1 error: 1) Spip\Archiver\Tests\SpipArchiverTest::testInformer with data set "empty-tar" (0, array(array(''), array()), '/Users/cedric/Sites/localhost...ty.tar', 'tar') RuntimeException: Directory name must not be empty. /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/src/TarArchive.php:44 /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/src/SpipArchiver.php:37 /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/tests/SpipArchiverTest.php:163 ERRORS! Tests: 20, Assertions: 49, Errors: 1. ``` En debugant je vois que si un fichier n'existe pas `$this->tar` n'est pas correctement initialisé dans le open() Le patch suivant corrige ce cas en verifiant que l'instantiation eu lieu correctement : ``` diff --git a/src/TarArchive.php b/src/TarArchive.php index 693c2ae..602d568 100644 --- a/src/TarArchive.php +++ b/src/TarArchive.php @@ -26,6 +26,10 @@ class TarArchive implements ArchiveInterface 2 ); + if ('' === $this->tar->getFilename()) { + return 0; + } + return 1; } @@ -38,7 +42,6 @@ class TarArchive implements ArchiveInterface if ('' === $this->tar->getPathname()) { return $files; } - $root_dir = dirname($this->tar->getPathname()); $source = new NoDotFilterIterator( new \RecursiveIteratorIterator( ``` Et du coup on voit que le test emballer sur le tar ne marche pas (c'était visiblement masqué par le fait qu'on avait pas d'erreur sur un fichier vide avant...) ``` ~/Sites/localhost/fraich40/plugins-dist/archiviste/ [master*] vendor/bin/phpunit (master|✚1) PHPUnit 9.5.13 by Sebastian Bergmann and contributors. Runtime: PHP 7.4.21 Configuration: /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/phpunit.xml.dist Warning: No code coverage driver available .F......F........... 20 / 20 (100%) Time: 00:00.030, Memory: 6.00 MB There were 2 failures: 1) Spip\Archiver\Tests\SpipArchiverTest::testInformer with data set "empty-tar" (0, array(array(''), array()), '/Users/cedric/Sites/localhost...ty.tar', 'tar') Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ Array ( 'proprietes' => Array ( - 'racine' => '' ) 'fichiers' => Array () ) /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/tests/SpipArchiverTest.php:166 2) Spip\Archiver\Tests\SpipArchiverTest::testEmballer with data set "tar" (true, 0, '/Users/cedric/Sites/localhost...er.tar', '', array('/Users/cedric/Sites/localhost...st.txt'), '/Users/cedric/Sites/localhost...ectory') compress ok Failed asserting that false matches expected true. /Users/cedric/Sites/localhost/fraich40/plugins-dist/archiviste/tests/SpipArchiverTest.php:283 FAILURES! Tests: 20, Assertions: 48, Failures: 2. ```

et ta modif fait planter les tests chez moi en PHP8.1 ... et en 7.4.27 (tu es en 7.4.21) et 8.0 aussi (dans une image docker)

Y aurait un compotement différend entre les versions ?

@JamesRezo il y a 2 sujets en fait : je pense que certaines version PHP étaient silencieuses sur le cas de l'archive foireuse et que en effet tu n'avais pas d'erreur dans ton cas.

Et une fois le patch ajouté qui teste que $this->tar->getFilename() renvoie bien quelque chose, du coup on voit qu'il y avait un autre test sur 'emballer un tar' qui échouait (echec qui était passé sous silence avant).

Donc moi aussi les tests sont en echec chez moi mais à cause du test testEmballer with data set "tar"

Le soucis, c'est que si les tests plantent, on peut se retrouver dans un état instable sur le jeu de test quand on relance phpunit...

Bon, je vais regarder ce que je peux faire sur les tar avec cette lib de mock de filesystem. Elle n'est pas compatible avec les Zip, mais on peut peut-être trouver un truc qui permettrait au moins de ne pas trimballer des fichiers tar/tgz dans le dépôt. À défaut d'apporter une solution immédiate au cas qui nous préoccupe, ça devrait au moins éviter l'instabilité. Par contre, pour trouver l'origine du comportement, ça va être plus compliqué...

PS @marcimat : tout ça pour des fonctions dont on se sert pas ^^ 😛

cela dit en effet il manque aussi une correction sur l'erreur attendue dans 1ad8dc4b98

~/Sites/localhost/fraich40/plugins-dist/archiviste/ [master*] git diff                                                                                                                                                        (master|✚1)
diff --git a/tests/SpipArchiverTest.php b/tests/SpipArchiverTest.php
index fbf9fbe..fd94ea6 100644
--- a/tests/SpipArchiverTest.php
+++ b/tests/SpipArchiverTest.php
@@ -127,7 +127,7 @@ class SpipArchiverTest extends TestCase
                 '',
             ],
 			'empty-tar' => [
-				0,
+				1,
 				[
                     'proprietes' => [],
                     'fichiers' => [],
                    ```
                    )

bon, c'est mort :

la lib de test de filesystem ne peut pas nous aider des masses pour le moment.
en fonction de la version de libzip (la lib C sur lequel s'appuie PHP pour la classe ZipArchive) on ne peut pas vider un fichier zip (comme pour tar avec zlib en gros). Du coup, selon comment est compilé PHP et l'extension zip, des fois, ça marche, des fois pas.

Donc, au mieux, on doit faire râler le plugin si on tente de retirer le dernier fichier d'une archive (et donc, ne plus traiter le cas empty-tar)

J'espère qu'un jour, quelqu'un utilisera cette fonctionalité (retirer des fichiers d'une archive) ! 😆

bah sinon on @deprecated retirer() et on throw une exception dessus… c’est ce que je voulais faire tout au début… avant de voir que ça ne semblait pas trop compliqué à faire.

Mais avoir une fonction non implémentée en api, ça me perturbait trop :)

57e4e11b1e

Bon, je crois que ça devrait être bon. ça fonctionne sur les 3 versions PHP ciblées.

C’est bon au fait.