Add Unit Tests to improve stability and avoid code regressions

[virtualtam]

The core of Shaarli is a big chunk of PHP: index.php.
It contains all the methods, which doesn't make editing and debugging convenient (lack of modularity). A good way to avoid bugs and regressions would be to implement unit testing, starting with sensitive features:

• data storage (read/write),
• authentication,
• import / export.

This could be done with a unit test framework like PHPUnit; see the awesome Kanboard project for an example implementation.

Some readings about the pros of unit testing everything:

• Is Unit Testing worth the effort? (Stack Overflow).

[nodiscc]

Unit tests would be great to have.

The core of Shaarli is a big chunk of PHP: index.php.

We could start by moving classes to a shaarli.class.php and include() it.

[virtualtam]

By experience, it's safer to first cover features with unit tests, then you can see what is broken / hidden when refactoring (in other words, "blind" refactoring, along with being a tedious task, is definitely not recommended...).

I think there is an historical & practical reason for Shaarli to be made of a single file, but, as it is now comprised of several resource (style, images, templates) subdirs, it would greatly benefit of having a more modular shape (a better readability makes spotting bugs and maintaining code way easier).

[e2jk]

I really like the idea of creating unit tests.

The original goal of @sebsauvage was indeed to have everything contained in
one single php file. That stopped being the case 3 years ago with:

• version 0.0.12beta [0] introducing jQuery files
• version 0.0.15beta [1] creating a separate CSS file
• version 0.0.28beta [2] adding the images folder
• The big one, version 0.0.32beta [3] using the templating engine

As far as I'm concerned, we're past the "fits in a single file" stage, and
this isn't mentioned anymore either on Github or Seb's wiki page.

I agree with creating the unit tests first, and then refactoring into
multiple files (those new PHP files should be put in a separate folder)

[0]
https://github.com/shaarli/Shaarli/tree/44a9d860e191878058072ab872be3a8e3339d21b
[1]
https://github.com/shaarli/Shaarli/tree/eae4f48bf0b4137ee4764d06678ba6e6a9c74978
[2]
https://github.com/shaarli/Shaarli/tree/6d946e78bfca94ac89494e84db3746473867c5ff
[3]
https://github.com/shaarli/Shaarli/tree/3433e5e8a84e3952502b79296f945e6dde2a7d75

[nodiscc]

Related #130 (comment):

A patch for a simple, initial test would be accepted.

[nodiscc]

#130 (comment)

Here's an efficient workflow, to bring tests to a software:

identify core / sensitive features,
cover them with tests, as relevant as possible:
nominal usage: what is a method supposed to do?,
nominal error handling: what is likely to break often?,
borderline cases: erroneous values, invalid arguments, etc.
this will likely highlight issues & flaws in the original code,
fix'em all!

Having a good coverage before doing any refactoring makes the process way easier: any new issue or behavior alteration will be instantly spotted!

Then, refactoring may happen as follows:

• think of a cool, elegant new design,
• update test code to fit the expected design
  • all tests will break!
• update / refactor the code until all tests are passing
  • again, issues may be spotted, covered and fixed.

Tests specify what the software is expected to perform, and how it is used, thus their importance ;-)
[...]
What unit and functional tests have brought:

• instant spotting of any regression,
• hyper-stable codebase,
• bugs we're facing are mostly minor, and can be fixed very quickly,
• easy refactoring,
• developer serenity ;-)

[...]
Oh, and the nice things about testing:

• writing them is not that hard, just time-consuming at first,
• it makes contributing simpler:
• for the contributor: test tell you if your contribution is good/stable enough for submission,
• for reviewers: you can see what has been brought by the PR, how it works,
• it won't prevent people from contributing; quite the opposite, actually: it shows you care about stability.

[nodiscc]

A few things to check for:

• Initialized but never used variables (see picwall: link directly to the target URL (not the permalink) #156)
• Attemps to write outside RAINTPL_TMP, DATADIR, CACHEDIR and PAGECACHE should not pass tests (Require write permissions only for relevant directories #40)
• @virtualtam More? I'm very new to unit tests, feel free to propose some (a PR is also welcome, it would allow us to experiment with this on a common branch)

[virtualtam]

Some test-able points:

• the easy one:
  • default class constructors;
• the functional one:
  • database interactions;
  • reading, writing, transactions;
  • parallel access (if any; the Python sqlite3 module allows multi-threaded connections, for instance);
• the security one:
  • session handling;
  • authentication;
  • password bruteforce / IP blacklist;
  • XSS.

And the way more advanced stuff (for the sole sake of awareness):

• template rendering;
• user interaction / browser automation: SeleniumHQ.

As a starter, I suggest writing tests for the linkdb class, which will provide hints on how to:

• write a test,
• test a class and its methods,
• check DB interactions,
• run tests ;-)

if it's OK, I'll start working on a PR to experiment unit test features; as usual, any feedback will be much appreciated :)

[nodiscc]

Yep, please proceed! A well commented test script would help. It's still a bit unclear to me how/what to test.

[mro]

I did a demo on integration tests (running against a webserver) over there: #16 (comment)

Maybe that's interesting to add (testing not multiple source distributions, but the current github code only).

What do you guys think?

P.S.: a driver to run the tests may be mink - see http://robbiemackay.com/2013/05/03/automating-behat-and-mink-tests-with-travis-ci/

[virtualtam]

@mro thanks for taking the first step towards functional testing ;-)

After a quick glance to phptestdemo, it seems to fairly increase the complexity of the Travis configuration, and will require a bit of tuning to get a minimal setup suitable to testing Shaarli.

Some comments & ideas:

• Docker migrated to a container-based infrastructure some times ago (see Travis: use the container-based infrastructure #290)
• the matrix may not be needed - Travis already runs a build on each branch + all PRs
• Nginx + PHP-FPM tend to be lighter to deploy than Apache2 and more flexible to configure
• though, supporting multiple web server configurations (Apache, Nginx, lighthttpd) could be a huge asset to tackle server/config-specific issues (sessions, headers, cookies, proxy...)

I have to admit I'm quite enthusiastic with experimenting functional testing, which could be introduced starting #333 - Refactor HTTP / Url functions (HTTP requests and response headers involved)

[mro]

@virtualtam things may not be so easy (when testing multiple shaarli versions as I want to do). See https://travis-ci.org/mro/phptestdemo/builds/78607719

The issues there - even with a extremely simple test - are:

• vanilla shaarli doesn't work with port other than 80 but travis + docker prevents sudo (and port numbers < 1024) for now,
• php 5.3 seems to lack web root command line switch

Both restrictions may be acceptable for this community shaarli. See https://github.com/mro/phptestdemo/tree/feature/docker .travis.ymland scripts/run-tests.sh for details.

[virtualtam]

Regarding the port limitation, I wonder if authbind could be used within a Travis docker appliance to allow serving on :80.

Example (provided chmod/chown are available):

touch /etc/authbind/byport/80
chmod 500 /etc/authbind/byport/80
chown <MY_PROCESS_USER> /etc/authbind/byport/80

[mro]

ok, pushed the integration tests a bit further, see #16 (comment)

[mro]

added an important test (for API #16, #113, #342, #130 when adding a link and currently sadly failing) in PR #354

[virtualtam]

Closing this generic thread as we're improving test coverage along code refactoring.