Les tests unitaires plantent #4417

Closed
opened 8 months ago by cerdic · 10 comments
cerdic commented 8 months ago
Owner

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. ```
Owner

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)

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)
Owner

Y aurait un compotement différend entre les versions ?

Y aurait un compotement différend entre les versions ?
Poster
Owner

@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"

@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"`
Owner

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 ^^ 😛

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](https://packagist.org/packages/mikey179/vfsstream). 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 ^^ 😛
Poster
Owner

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' => [],
                    ```
                    )
                    
cela dit en effet il manque aussi une correction sur l'erreur attendue dans 1ad8dc4b98beee8de6e40c16e1748fc66910358e ``` ~/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' => [], ``` )
Owner

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) ! 😆

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) ! 😆
Owner

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 :)

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 :)
Owner
https://git.spip.net/spip/archiviste/commit/57e4e11b1e04c02ea6d3d4e027eaa3257986f976
Owner

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

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

C’est bon au fait.

C’est bon au fait.
marcimat closed this issue 8 months ago
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.