LinkDB: move to a proper file, add test coverage

[virtualtam]

Hi all!

This is a first polished draft to bring unitary tests to Shaarli 😄

To help revewing the PR, and bring some logic to the test-writing process, I've split this comment in 3 sections:

• the bare commit message: what's been done
• how to configure your dev environment to run tests (as well as syntax analysis, aka lint)
• implementation details
  • technical choices and assumptions
  • coding guidelines followed

As usual, feel free to comment the PR, make suggestions, ask for explanations... ^^

Commit message

LinkDB: move to a proper file, add test coverage

Relates to #71

LinkDB
 - move to application/LinkDB.php
 - code cleanup
   - indentation
   - whitespaces
   - formatting
 - comment cleanup
   - add missing documentation
   - unify formatting

Test coverage for LinkDB
 - constructor
 - public / private access
 - link-related methods

Shaarli utilities (LinkDB dependencies)
 - move startsWith() and endsWith() functions to application/Utils.php
 - add test coverage

Dev utilities
 - Composer: add PHPUnit to dev dependencies
 - Makefile:
    - update lint targets
    - add test targets
    - generate coverage reports

Setup your environment for tests

The framework used is PHPUnit; it can be installed with Composer, which is a dependency management tool.

Regarding Composer, you can either use:

• a system-wide version, e.g. installed through your distro's package manager
• a local version, downloadable here

Sample usage

# system-wide version
$ composer install
$ composer update

# local version
$ php composer.phar self-update
$ php composer.phar install
$ php composer.phar update

Install Shaarli dev dependencies

$ cd /path/to/shaarli
$ composer update

Install and enable Xdebug to generate PHPUnit coverage reports

For Debian-based distros:

$ aptitude install php5-xdebug

For ArchLinux:

$ pacman -S xdebug

Then add the following line to /etc/php/php.ini:

zend_extension=xdebug.so

Run unit tests

$ make test

Development notes

The main aspects of the PR are to:

• move the LinkDB class from index.php to its own file
• add test coverage for its methods
  • nominal usage cases
  • some corner cases as well ;-)

Syntax

I've tried to stick to the PHP Standard Recommendations (PSR), especially:

• PSR-1 - Basic Coding Standard
• PSR-2 - Coding Style Guide

To this effect:

• missing comments have been added
• all comments have been unified to follow the same style
• code syntax has been unified to follow the same style
  • indentation
  • spacing
  • braces, brackets

Structure

# Shaarli classes
application/
├── LinkDB.php
└── Utils.php

# Shaarli unitary & functional tests
tests/
├── LinkDBTest.php
├── utils  # utilities to ease testing
│   └── ReferenceLinkDB.php
└── UtilsTest.php

# Shaarli's main page
index.php
[...]

As stated in a previous comment, classes have been moved under the application directory; feel free to suggest a more appropriate or explicit location (e.g. lib, classes, shaarlib...)!

[nodiscc]

This is great, thank you.
I did not have the time to test this patch yet, will merge as soon as possible.
Any other reviews are welcome.

[virtualtam]

@nodiscc you're welcome ;-)

IMO, this PR is not yet ready for being merged, as some aspects and conventions need to be defined. I've added a bunch of TODO and FIXME comments within the code as well, that suggest future fixes and improvements:

• variable & method naming
• access to global variables
• usage of exceptions instead of exit() and die() calls
• unitary tests' structure
• ...

[ArthurHoaro]

I agree with @nodiscc, this is really great.

I didn't really review the code itself, but it looks fine to me. Your TODOs in LinkDB are mostly improvements and shouldn't really be related to this PR.

And according to coverage, this is definitely a good start:

2 notes:

• be careful with Do not load links if they're hidden (also fix shaarli/Shaarli#202) #222 conflicts.
• tests are not compatible with PHP 5.3.

[virtualtam]

be careful with #222 conflicts.

No problem with #222, this PR will be rebased on master as long as needed

tests are not compatible with PHP 5.3.

5.3 is a bit old, though :-)
I noticed it when testing #219, maybe this could be added as a future, nice-to-have improvement?

And according to coverage, this is definitely a good start

HTML Coverage reports are definitely a plus, they're very convenient to browse for information

Your TODOs in LinkDB are mostly improvements

I usually don't add TODOs inside code when an issue tracker is available, but given the size of the PR, they may prove helpful when reviewing (think of them as memos)

[TODOs] shouldn't really be related to this PR.

• they are, in the sense that they've been highlighted by refactoring, or the impossibility to properly test some code; whether they are valid or not is in the scope of this PR
• they are not, in the sense that they won't be fixed right now, rather in future PRs

[ArthurHoaro]

5.3 is a bit old, though :-)

I know and I don't think it's important, but it is worth mention it. It's probably easy to fix if we want to though.

HTML Coverage reports are definitely a plus, they're very convenient to browse for information

Yep. I've just add --coverage-html testhtml in the Makefile. Note that xdebug need to be enabled.

they are not, in the sense that they won't be fixed right now, rather in future PRs

Yes, that's what I meant. :)

So, what do you think this PR needs to be "mergeable"?

[virtualtam]

I think I'll add a makefile target to generate the coverage report, and proof-read the code for typos; if everyone's OK with the current structure (classes going under application/, test data), then this should be a good candidate for going to master

[virtualtam]

Test coverage added, 2 reports are generated after running tests (provided Xdebug is installed & enabled):

• an overall summary on the console
• a full HTML report under coverage/

The configuration takes place in phpunit.xml, to filter sources and customize the generated output, including color tresholds for fanciness:

• c < 30% => bad coverage
• 30% < c < 80% => medium coverage
• c > 80% => good coverage

[nodiscc]

2 reports are generated after running tests

also great.

I've added a bunch of TODO and FIXME comments within the code as well,
Your TODOs in LinkDB are mostly improvements and shouldn't really be related to this PR.
given the size of the PR, they may prove helpful when reviewing (think of them as memos)

Your TODOs are very helpful and could be split in 2 categories
Shaarli main code related TODOs that could be added as separate issues:

• TODO: use exceptions instead of "die" (in several places)
• TODO: raise an exception if the file is not write-able in checkDB
• TODO: check input format in filterDay
• variable & method naming; access to global variables

TODOs related to tests improvements, could be added as items in #71

• TODO: move PageCache to a proper class/file in testSaveDB
• TODO: check input format in testFilterInvalidDay

tests are not compatible with PHP 5.3.
5.3 is a bit old, though :-)

I would also be ok with no PHP 5.3 compatibility

this PR will be rebased on master as long as needed

Rebasing large commits several times is cumbersome - though I just discovered N’arbitrez vos conflits Git qu’une fois grâce à rerere which looks excellent - so I'd be ok to merge this, when it is confirmed that is it better than what we have? = yes and does it introduce bugs? = probably not.

A question that comes to mind is: can the test methods be manipulated to malicious purposes when accessible on an Internet-facing Shaarli (data disclosure, data loss, filling up disk space with test results...). How do we test tests?

[virtualtam]

PR updated with .htaccess files for new directories

By the way, when using Apache2, a lot of non-PHP resources are available through a browser, should we raise an issue to add a proper root .htaccess to:

• prevent accessing restricted resources
• redirect users to index.php (may require mod_rewrite)

A question that comes to mind is: can the test methods be manipulated to malicious purposes when accessible on an Internet-facing Shaarli

Hence the importance of ensuring test code is not reachable outside the CLI. I can test this PR against the following servers on Linux:

• Apache2 / httpd
• nginx

Do you know of other (commonly used) servers that would be subject to access vulnerabilities and would require further testing?

data disclosure, data loss, filling up disk space with test results...

By nature, test code should never, ever, access or depend on production data. Enforcing this helps a lot in detecting unsafe, poorly written and/or untestable code.

How do we test tests?

i've been wondering this for some time now, with no satisfying answer but this general reasoning:

• a couple tests are better than no test at all
• simple, clear code is often testable with simple, clear tests
• code with no external or hidden dependencies (globals, environment...) should be preferred (functional programming-inspired approach)
  • easier to read (every dependency is explicit), thus to edit and fix
  • accurately testable with different input sets
  • safer to run (minimized side-effects)

[nicolasdanelon]

+1 for this pull request
What if we add a prod/dev flag? if prod not test else test
@virtualtam @nodiscc @ArthurHoaro :)

[nicolasdanelon]

Just to know what all of you think about..
When you download prestashop from the website all the tests are in production, why this can happen in Prestashop and not in Shaarli ?

[virtualtam]

Thanks @nicolasdanelon for the link! It's good to have one more example for implementing tests and structuring code :)

What if we add a prod/dev flag? if prod not test else test

Files and dirs can be excluded from deliverables, e.g. using a .gitattributes file, see Use gitattributes to keep your tests out of other people's production

all the tests are in production

Having test code provided along production code doesn't mean that test code is reachable from production servers.

why this can happen in Prestashop [...]

There are (at least) two ways to perform this, with a similar end result:

• server-side: rewrite URLs
• application-side: dispatch, or route URLs

Here's an example for Shaarli:

• allow access to all static resources: images, favicon, stylesheets, Javascript...
• allow access to cached resources (thumbnails, images)
• optionally, allow access to documentation
• redirect all other requests to index.php

Some rewriting modules for popular Web servers:

• Apache: mod_rewrite
• lighthttpd: mod_rewrite
• nginx: ngx_http_rewrite_module

Some web frameworks providing routing features:

• Django
• Symfony

After a quick glance at PrestaShop's source code and deliverables, it looks like it uses a custom URL dispatcher, see classes/Dispatcher.php

[...] and not in Shaarli?

TL;DR: may not be a priority for now (pre-1.0)

Both software can hardly be compared:

• they differ in size and resources by several orders of magnitude, as well in community goals:
  • PrestaShop provides an e-payment platform for shops, thus has huge security, stability, performance and maintainability constraints
  • Shaarli is a personal, minimalist, super-fast, no-database delicious clone, it's aimed at storing one's personal data, and for now is a bit loose on security: no LDAP, no CAS, no SSO, no proper database backend, resources being reachable in production...

While being kept secure, Shaarli must stay simple to setup, and server dependencies (packages, modules, configuration) should be avoided (users may not have access to them).

A couple well-thought .htaccess templates for Apache, as well as example configuration for other Web servers, could represent a good trade-off to start with.

[nicolasdanelon]

Awesome answer :) thanks @virtualtam

[nodiscc]

Will merge on sunday 21.

.htaccess looks like a sufficient access restriction method, as Shaarli has no router. nginx/non-apache users may want to chip in and provide instructions on how to set up equivalent access restrictions for their setup

[ArthurHoaro]

Also keep in mind that tests are contained in PHP classes which are never instanciated.

[virtualtam]

OK for merging, I'll take time to write a guide to running & writing tests :)

[nodiscc]

Thanks! Merged.

[nodiscc]

Documentation updated, thanks also for writing a clear summary.