Skip to content

Improve atoum test detection in file #82

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jan 24, 2018

Conversation

vdechenaux
Copy link
Member

Fixes #75

I reproduced the issue (2 classes in a file, 1st is not a test, 2nd is a test class).

This patch improve the test detection in a file.
It doesn't always takes the first class, and it checks if the class extends atoum

@Grummfy
Copy link
Member

Grummfy commented Feb 27, 2017

if you create a base test class A that extends atoum and inside your test class use this A class, I'm not sure it will works ;(

@vdechenaux
Copy link
Member Author

Do you mean something like this, with two files?

namespace XXX\Video\tests\units;
class VideoRepository extends FunctionalTest
namespace XXX\Test\Util;
abstract class FunctionalTest extends \atoum

This case is workig :)

@Grummfy
Copy link
Member

Grummfy commented Feb 27, 2017

oki, perfect.

// We also check if the class extends atoum
while (checkedClass.getSuperClass() != null) {
PhpClass parent = checkedClass.getSuperClass();
if (parent.getFQN().equals("\\atoum")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dans certains cas la détection ne fonctionne plus.
Par exemple :

namespace Pickle\tests\units\Engine;

use atoum;
use Pickle\tests;

class PHP extends atoum

Il serait utile de de vérifier aussi que le nom de la classe étendue soit "atoum".
et/ou ne faire ce test que si il y a plus d'une classe dans le fichier.

Copy link
Member Author

@vdechenaux vdechenaux Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tu as eu le soucis en live, en compilant le pluggin avec le code de cette branche, ou c'est une supposition?
J'ai des classes de tests qui sont dans ce cas là et tout se passe bien.

Il serait utile de de vérifier aussi que le nom de la classe étendue soit "atoum".

C'est bien ce que fait le equals sur le FQN

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, ok, je viens bien de reproduire le problème...
Ca ne marche pas sur le projet que tu as cité en effet.
C'est parce que phpstorm ne connais pas la classe \atoum et du coup ne la fait pas remonter dans getSuperClass.
Je testais depuis le début sur un projet qui a atoum/stubs.

Je vais regarder ca

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cas corrigé, avec ajout d'un test

@vdechenaux
Copy link
Member Author

Si #88 est mergée avant celle ci, j'ajouterais des tests pour les différents cas

@vdechenaux
Copy link
Member Author

@agallou Voila quelques tests!

@@ -1,6 +1,6 @@
sudo: false
language: java
dist: precise
dist: trusty
Copy link
Member Author

@vdechenaux vdechenaux Sep 25, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Le build passait pas sur precise, y avait un probleme avec Ant

@vdechenaux
Copy link
Member Author

I re reviewed it, I think it's ok.
New tests in 1fa6357 give more confidence

@vdechenaux vdechenaux merged commit 4957ac4 into atoum:master Jan 24, 2018
@vdechenaux vdechenaux deleted the fix-test-detection branch January 24, 2018 18:43
vdechenaux added a commit that referenced this pull request Oct 3, 2018
Tests should extends \atoum (cf doc) but you can also extends \atoum\test and it was not supported since #82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants