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

Match YOURLS branch with Unit test branch #2301

Merged
merged 6 commits into from Oct 7, 2017

Conversation

Projects
None yet
3 participants
@ozh
Member

ozh commented Sep 30, 2017

Some refactoring of YOURLS code needs change in the unit tests as well. Currently I commit to unit tests master and run tests for a YOURLS branch. This is lame.

Goal:

  • check YOURLS branch being tested
  • if YOURLS/YOURLS-unit-tests has a branch with the same name, checkout this branch

ozh added some commits Sep 30, 2017

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Sep 30, 2017

Member

Or maybe move back these tests to the main repository? 😇
(cf mail du lundi 14 avril 2014 à 16:27, ouais ça date)

Edit: (pour suivre ta réponse du 6 nov. 2014, en la relisant)

  • I'm not sure it really relevant to promote git deployment for "low-level" users. They MUST install released versions for stability. Easy for us to support and to be flexible with branches.
  • Regarding database warnings, everything is available both with PHP and on Travis to be sure we are in the perfect environment to run tests.
Member

LeoColomb commented Sep 30, 2017

Or maybe move back these tests to the main repository? 😇
(cf mail du lundi 14 avril 2014 à 16:27, ouais ça date)

Edit: (pour suivre ta réponse du 6 nov. 2014, en la relisant)

  • I'm not sure it really relevant to promote git deployment for "low-level" users. They MUST install released versions for stability. Easy for us to support and to be flexible with branches.
  • Regarding database warnings, everything is available both with PHP and on Travis to be sure we are in the perfect environment to run tests.
@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Sep 30, 2017

Member

@LeoColomb hmm c'était quoi les arguments pour & contre à l'époque?

Member

ozh commented Sep 30, 2017

@LeoColomb hmm c'était quoi les arguments pour & contre à l'époque?

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Sep 30, 2017

Member

By @LeoColomb on Apr. 12, 2014 12:41 (j'étais encore jeune 😉)

De même [que pour la doc], la logique serait de conserver les tests unitaires dans le repository de YOURLS.
L’argument très juste que tu m’as proposé au début pour les séparer était de ne pas les inclure dans le package que les utilisateurs téléchargent.
Depuis la révélation du .gitattributes, on peut les exclure facilement au téléchargement.
L’intérêt ? Inciter à créer des tests sur toute modification du script, sans être obligé de passer par deux repo qui compliquent la synchronisation.

By @ozh on Apr. 14, 2014 16:27

Pas bête. Effectivement. Je t'avoue qu'instinctivement ma réaction c'est "oulla non" mais en y réfléchissant je ne vois pas pourquoi on ne le ferait pas. C'est quand même bien genial ce gitattribute :)

Pour ceux qui déploient par git, il faudra suggérer un script type qui "git pull && rm -rf unit-tests" pour la sécurité de l'installation. On pourrait aussi inclure une notice dans l'interface admin si on détecte le répertoire unit-tests.

Oué, très bonne idée. Vendu.

By @ozh on Nov. 06, 2014 18:27

Concernant les unit tests j'y ai réfléchi récemment en m'y replongeant dedans et finalement ça ne me parait pas une bonne idée de l'intégrer à YOURLS.

Il y a un risque très fort d'avoir un répertoire /tests dans un site live (a minima tous ceux qui déploient via git) ce qui aura 2 conséquences fâcheuses :
Il y a un risque très fort d'avoir un répertoire /tests dans un site live (a minima tous ceux qui déploient via git) ce qui aura 2 conséquences fâcheuses :

  1. d'abord, un problème de "Full Path Disclosure"
    https://www.owasp.org/index.php/Full_Path_Disclosure
    En testant http://sho.rt/tests/includes/bootstrap.php ou tout autre fichier on aura un beau message d'erreur avec le chemin complet
    Fatal error: Class 'PHPUnit_Framework_TestCase' not found in C:\Users\ozh\www\YOURLS-unit-tests\includes\utils.php on line 118

  2. Mais surtout, surtout,
    les tests sont destructifs au niveau de la base de données: on efface tout, on insère des URLs, on efface des URLs
    Le risque est trop grand qu'un utilisateur purge sa base en prod parce qu'il aura mal configuré ou tout simplement pas compris ce qu'est ce répertoire tests. J'ai purgé comme un benêt la base sur mon install de test à la maison, en réalisant avec horreur quelques minutes plus tard que punaise, ouf, ça n'est que ma base de test

Member

LeoColomb commented Sep 30, 2017

By @LeoColomb on Apr. 12, 2014 12:41 (j'étais encore jeune 😉)

De même [que pour la doc], la logique serait de conserver les tests unitaires dans le repository de YOURLS.
L’argument très juste que tu m’as proposé au début pour les séparer était de ne pas les inclure dans le package que les utilisateurs téléchargent.
Depuis la révélation du .gitattributes, on peut les exclure facilement au téléchargement.
L’intérêt ? Inciter à créer des tests sur toute modification du script, sans être obligé de passer par deux repo qui compliquent la synchronisation.

By @ozh on Apr. 14, 2014 16:27

Pas bête. Effectivement. Je t'avoue qu'instinctivement ma réaction c'est "oulla non" mais en y réfléchissant je ne vois pas pourquoi on ne le ferait pas. C'est quand même bien genial ce gitattribute :)

Pour ceux qui déploient par git, il faudra suggérer un script type qui "git pull && rm -rf unit-tests" pour la sécurité de l'installation. On pourrait aussi inclure une notice dans l'interface admin si on détecte le répertoire unit-tests.

Oué, très bonne idée. Vendu.

By @ozh on Nov. 06, 2014 18:27

Concernant les unit tests j'y ai réfléchi récemment en m'y replongeant dedans et finalement ça ne me parait pas une bonne idée de l'intégrer à YOURLS.

Il y a un risque très fort d'avoir un répertoire /tests dans un site live (a minima tous ceux qui déploient via git) ce qui aura 2 conséquences fâcheuses :
Il y a un risque très fort d'avoir un répertoire /tests dans un site live (a minima tous ceux qui déploient via git) ce qui aura 2 conséquences fâcheuses :

  1. d'abord, un problème de "Full Path Disclosure"
    https://www.owasp.org/index.php/Full_Path_Disclosure
    En testant http://sho.rt/tests/includes/bootstrap.php ou tout autre fichier on aura un beau message d'erreur avec le chemin complet
    Fatal error: Class 'PHPUnit_Framework_TestCase' not found in C:\Users\ozh\www\YOURLS-unit-tests\includes\utils.php on line 118

  2. Mais surtout, surtout,
    les tests sont destructifs au niveau de la base de données: on efface tout, on insère des URLs, on efface des URLs
    Le risque est trop grand qu'un utilisateur purge sa base en prod parce qu'il aura mal configuré ou tout simplement pas compris ce qu'est ce répertoire tests. J'ai purgé comme un benêt la base sur mon install de test à la maison, en réalisant avec horreur quelques minutes plus tard que punaise, ouf, ça n'est que ma base de test

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Sep 30, 2017

Member

And I would add, today in 2017, that is now very common to have test inside the repository for consistency (as this PR raise the problem).
The security issue is a wrong one IMHO: as I said some comment before, git deploy is NOT a way for usual users, whom should follow release packages.

😃

Member

LeoColomb commented Sep 30, 2017

And I would add, today in 2017, that is now very common to have test inside the repository for consistency (as this PR raise the problem).
The security issue is a wrong one IMHO: as I said some comment before, git deploy is NOT a way for usual users, whom should follow release packages.

😃

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Oct 1, 2017

Member

Tests inside the repo for libraries, for sure, but for whole web applications as well?

Member

ozh commented Oct 1, 2017

Tests inside the repo for libraries, for sure, but for whole web applications as well?

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Oct 1, 2017

Member

Yes, this is now a standard even for final applications. With strong arguments for maintainability and reliability.

Member

LeoColomb commented Oct 1, 2017

Yes, this is now a standard even for final applications. With strong arguments for maintainability and reliability.

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Oct 1, 2017

Member

@LeoColomb Any example of web apps repo with tests?

Member

ozh commented Oct 1, 2017

@LeoColomb Any example of web apps repo with tests?

@dgw

This comment has been minimized.

Show comment
Hide comment
@dgw

dgw Oct 1, 2017

Collaborator

Many. I've been working with https://github.com/thelounge/lounge lately (Node) and npm test is part of the pre-PR process. (Their test scripts also lint the JS and CSS files.)

I didn't realize YOURLS tests weren't in the repo until now. Thought that was pretty standard.

Collaborator

dgw commented Oct 1, 2017

Many. I've been working with https://github.com/thelounge/lounge lately (Node) and npm test is part of the pre-PR process. (Their test scripts also lint the JS and CSS files.)

I didn't realize YOURLS tests weren't in the repo until now. Thought that was pretty standard.

@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Oct 2, 2017

Member

Why not then... Next branch will test moving tests inside this repo.

Member

ozh commented Oct 2, 2017

Why not then... Next branch will test moving tests inside this repo.

@LeoColomb

This comment has been minimized.

Show comment
Hide comment
@LeoColomb

LeoColomb Oct 2, 2017

Member

Why not then

🎉

Next branch

What do you mean?

Member

LeoColomb commented Oct 2, 2017

Why not then

🎉

Next branch

What do you mean?

Show outdated Hide outdated .travis.yml
@ozh

This comment has been minimized.

Show comment
Hide comment
@ozh

ozh Oct 3, 2017

Member

@dgw Unrelated: per @LeoColomb's suggestion I've added you as a "member of the team", or a "collaborator", not sure what I've done :) You've been providing several very neat answers and feedback on issues, feel free to keep going, close trivial stuff and generally do what you'd like to do :)

Member

ozh commented Oct 3, 2017

@dgw Unrelated: per @LeoColomb's suggestion I've added you as a "member of the team", or a "collaborator", not sure what I've done :) You've been providing several very neat answers and feedback on issues, feel free to keep going, close trivial stuff and generally do what you'd like to do :)

@dgw

This comment has been minimized.

Show comment
Hide comment
@dgw

dgw Oct 4, 2017

Collaborator

@ozh & @LeoColomb, I'm honored by the invitation. Didn't realize I'd been doing anything that noticeable! I will graciously accept, and keep doing what I can to cut down on trivial/support issues, etc.

Now, back to your regularly scheduled code review. 😁

Collaborator

dgw commented Oct 4, 2017

@ozh & @LeoColomb, I'm honored by the invitation. Didn't realize I'd been doing anything that noticeable! I will graciously accept, and keep doing what I can to cut down on trivial/support issues, etc.

Now, back to your regularly scheduled code review. 😁

Remove [Remove xdebug]
We don't care about speeding tests up. [skip ci]

@ozh ozh merged commit 8cbd20f into master Oct 7, 2017

0 of 2 checks passed

Scrutinizer Running
Details
codacy/pr Hang in there, Codacy is reviewing your Pull request.
Details

@ozh ozh deleted the checkbranch branch Oct 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment