Platform Unittests

[Hannes Papenberg]

Hi folks,
about 6 months ago I started looking at the unittests for the platform
and tried to improve those. While I did my work, I noticed that the
tests are not in the best shape... And I don't mean the missing/skipped
tests, but the structure of our test folders. A good example are our
database tests. When I started, we had JDatabaseMySQL and a
corresponding JDatabaseMySQLTest class. Then it was rewritten to
JDatabaseDriverMySQL and the class was moved to another folder in the
platform. The unittest however stayed in the same folder and while the
class was renamed, the file wasn't. Now the tests for the database
drivers are apparently in a seperate folder outside of the whole
structure of the unittests. The im- and exporters however are still
"tested" in the normal/common structure of our tests.

This is just an example of several issues in that regard that I found
and I want to ask two things:
1. Is there a good reason to create these special cases, like renamed
classes, but not renamed tests and especially moving the DB testing
partially out of our normal testing structure?
2. Please, when refactoring, don't just make the tests somehow work, but
do it properly, adding all methods at least as stubs that you added and
change both classnames AND filenames, so that we are not left with a
JDatabaseMySQLTest without a corresponding JDatabaseMySQL.

I'm happy to help out in getting the tests up to speed again and to
improve them, but in that process I also have to ask the people managing
the pull requests for the platform to respond to these requests more
quickly. In my work I discovered about 100 classes that were not covered
even by unittest stubs and thus didn't even show up in the code coverage
report at all. I prepared pull requests for that and am now in the
process of preparing those requests for the 4th time, since the others
were not approved in time before some major refactoring made the merge
break again, often enough breaking it so much that it was easier for me
to start over than to try to salvage the wreckage.

Thanks for your time.

Hannes

[Hannes Papenberg]

In addition to this, could someone please publish the link to our
Jenkins installation (http://build.joomla.org) and the link to the
platform code coverage report in the documentation?

Thanks

Hannes

[Gary Glass]

OK so I admit I'm don't have time to stay plugged in to J! dev or platform
or CMS, but I really think it would be a good idea to have a standard (and
published) checklist of requirements for getting a pull request approved for
production: comments, unit tests, coverage, code convention conformance,
etc. Joomla needs to find its way out of the dark forest of regression
errors. Yes it's volunteer, but that doesn't mean it has to be amateurish. A
lot of people are trying to make a living on this code base.

______________________________
Gary Glass
http://ShutterGlass.com
http://OnlyBegotten.com

Thanks

Hannes

--
I am using the free version of SPAMfighter.
We are a community of 7 million users fighting spam.
SPAMfighter has removed 266 of my spam emails to date.
Get the free SPAMfighter here: http://www.spamfighter.com/len

The Professional version does not have this message

[Louis Landry]

@Gary,

Nice shot across the bow.  As far as expectations for inclusion, they've been published for a while now: http://developer.joomla.org/strategy.html#stabletrunk.  Could they be improved?  Sure.  There is something there though.  Additionally, nothing goes through from the Joomla Platform staging branch to master without passing an automated suite that checks code style with PHPCS as well as validates that all the unit tests pass.  You characterization that Joomla is in a dark forest of regression is a bit much.  There is a lot to improve upon form an API standpoint, and that is a driving factor for separating the platform code out to be worked on independently, but let's try to avoid the hyperbole please.

@Hannes,

1. Is there a good reason to create these special cases, like renamed classes, but not renamed tests and especially moving the DB testing partially out of our normal testing structure?

For your first query, there was a reason; whether or not it was good is up to interpretation.  The idea was that by not changing the unit tests we served a better chance of ensuring that the massive refactoring of the database package had a higher level of backward compatibility.  Rewriting the unit tests for the database package would have made it more likely that something would have been missed.  This was always a temporary measure to demonstrate that it all worked.

The second part of your question gets to a more fundamental aspect of unit testing.  Ultimately, we have lots of tests that are in fact not "unit" tests.  Many of these depend on a database to run.  In a perfect world we'd have good seams where we can mock out the database and just test the "units" of code that we are writing the tests for.  I'm hoping that over time we can separate the true "unit" tests from "integration" tests.  Integration tests would be tests that depend on external systems to run, such as database tests.

That, however, won't answer your question as to why the JDatabaseDriver* tests were separated from the rest of the test suite.  The concept here is that the "unit" suite now runs completely against an in-memory SQLite database that is created and loaded at runtime.  Everything in the "unit" suite can be run completely configuration-free.  The things in the database suite all require connections to external database systems, and would require environment specific configuration to run.

2. Please, when refactoring, don't just make the tests somehow work, but do it properly, adding all methods at least as stubs that you added and change both classnames AND filenames, so that we are not left with a JDatabaseMySQLTest without a corresponding JDatabaseMySQL.

So this notion that we have to have skeletons and stubs for everything in the platform is actually not accurate in my opinion.  It is quite possible to have PHPUnit simply add all the files without cluttering up the test tree with a bunch of meaningless skeleton files.

For reference: http://www.phpunit.de/manual/current/en/appendixes.configuration.html#appendixes.configuration.blacklist-whitelist

With respect to the notion of fixing up the tests when you change code, in general I'm fully supportive.  We took this slightly different approach with the database package for the reasons I've outlined above temporarily. Hopefully now that we've demonstrated a very high level of backward compatibility we can move forward and revamp all the unit tests for the database package.  In the ideal case we here we'd be mocking out the database system dependencies completely in the unit suite, and only putting the very specific parts where we need to test integration with the database systems in the separate "database" suite.

Cheers,

- Louis

[Elin Waring]

The real thing is that we need to get Nikolai's excellent pull tester (which is much better than anything on the build site) integrated and make those results available via github. I really suggest just book marking the site in your browser if you are someone who needs it on a regular basis. 

 One issue I am concerned about is that I see changes going in without appropriate updates/additions to tests.  It's not enough just to say that the existing tests don't break, and really by definition a bug implies that there was a missing failing test. 

A larger base issue is that we have actually created something of a disincentive to create failing tests. I bumped up against this the other day because in proposing some new code (to detect Android tablets) I discovered that browser version detection for some Opera UA strings was failing. This wasn't really about testing my code, and I could just have not included that string in the tests that went with my proposed code and had everything pass with flying colors---but is that what we want? I don't think so.   But with the no failing tests rule reporting a failing test  actually makes it less likely that your code will ever be committed.   Do we have a situation where creating/reporting a failing test without code to fix it is unintentionally discouraged? I think we may. 

Elin

[Hannes Papenberg]

Hi Louis,
some of your answers are not entirely clear to me. I'll try to rephrase
them and maybe you can say if I'm correct or not:

1. Moving/rewriting the unittests for the JDatabase class was not done
to make sure that the whole change to the DB systemprocess is backwards
compatible. (simply because the old unittests still worked) Would now be
the time to do that rewrite/change?

2. "unit"tests vs. "integration"tests: We want to seperate all the
integration tests into their own branch. If it were up to me, I wouldn't
do that, since I don't necessarily see the benefit. But we especially
now only have the JDatabaseDriver* classes outsourced like this. What
about JDatabaseExporter*, JDatabaseImporter*, JCacheStorage*,
JSessionStorage* and JGithub*? All those classes depend on external
services and subsequently would have to be moved into that branch. Maybe
a better approach would be to define some rules how to layout such tests
in our current structure, which would also help with our other tests,
since they use several different ways to store mock and inspectore classes.

3. "We don't need unittest stubs for our test tree, we can always simply
generate them on the fly with phpunit" If you mean only the code
coverage report, I might agree with you, but I see a few more issues
with not having the stubs around. First of all, our test report would
incorrectly state that all tests have been implemented, and second of
all, someone who wants to write a unittest for an existing class then
can't simply tackle that single method that he/she would have written
before, but also has to generate the stub first and get that one
working. And no, the stubs generated by PHPUnit don't work all right out
of the box. So I strongly suggest to get our unittest base fixed up now
and not postpone that to the point where someone might want to write a
test...

Hannes

[Hannes Papenberg]

You know that Nikolai pulled the plug on the pulltester some time ago,
right?

Hannes

[CirTap]

On Tuesday, March 20, 2012 10:22:26 AM UTC+1, Elin Waring wrote:

The real thing is that we need to get Nikolai's excellent pull tester (which is much better than anything on the build site) integrated and make those results available via github.

are you refering to this one? http://elkuku.github.com/pulltester/

CirTap

 

[CirTap]

> The real thing is that we need to get Nikolai's excellent pull tester

are you refering to this one? http://elkuku.github.com/pulltester/

CirTap

[Michael Babker]

Speaking of the pull tester, is joomla-jenkins still working?  I haven't seen anything on GitHub from that one lately…

From: Elin Waring <elin....@gmail.com>
Reply-To: <joomla-de...@googlegroups.com>
Date: Tue, 20 Mar 2012 02:22:26 -0700 (PDT)
To: <joomla-de...@googlegroups.com>
Subject: [jplatform] Re: Platform Unittests

[Rouven Weßling]

Jools has disappeared for the time being, AFAIK Ian's working on bringing it back.

Rouven

[Rob Schley]

Nope, it's broken. Needs to get fixed.