412083 - Unit Tests For Bugzilla

Status: NEW

(Bugzilla :: Testing Suite, enhancement)

Description

[Max Kanat-Alexander]

The basic idea of unit tests is:

Add tests for every method in every object. Check: Does this method do what it's supposed to do?

For example, you could add a unit test for $bug->add_group. Does it correctly update the Bug object when a valid group is added? Does it correctly throw an error when you try to add an invalid group?

There are various modules in the Test:: namespace on CPAN that help with this sort of testing.

In order to do this, we need to be able to have a test database that is separate from the real database. What I'm thinking is that we can add SQLite support to Bugzilla, and then have the tests:

1) Create a test database before starting the unit tests.
2) Populate that test database based on some variables set in constants
   in a Bugzilla::Test package (or something like that).

There may also be unit testing frameworks for Perl that we could use, although I know nothing about that. (The standard testing framework for Perl is Test::Harness, of course, and most people use Test::More on top of that, but there might be additional modules or frameworks we can use for writing unit tests.)

Comment 1

[Max Kanat-Alexander]

Barring any other unit test framework, I was thinking we would have modules in the Bugzilla::Test namespace named after the standard modules, and we would have functions named after the methods we were going to test.

So, for example, to test Bugzilla::Bug::create, we would have a function called Bugzilla::Test::Bug::create.

The Bugzilla::Test modules would live under t/, I was thinking, not under our normal Bugzilla/ directory.

Comment 2

[Frédéric Buclin]

FYI, the QA team already has tests for each webservice method available. We don't test each specific object method, though.

Comment 3

[irina]

Attachment: Bugzilla Unit Tests Using Test::More

My first attempt to start with the project. Could you give some comments? Thanks.
http://zenit.senecac.on.ca/wiki/index.php/Bugzilla_Unit_Tests_Using_Test::More

Comment 4

[irina]

Attachment: Mockup Objects for Keyword.t

Comment 5

[Max Kanat-Alexander]

Comment on attachment 343049 [details]
Mockup Objects for Keyword.t

You should not have to have your own Bugzilla::DB, etc. You should just be using the normal ones--otherwise you're not testing our real code.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 343048 [details]
Bugzilla Unit Tests Using Test::More

General comments:

Perhaps it might be a good idea to create a Bugzilla::MockObject, a subclass of Test::MockObject (or some similar CPAN class) for this.

Instead of creating a DB manually and all that, I think that we should just have a separate localconfig.test or something, that checksetup.pl can create with a --test-db argument or something like that. Then we can have a USAGE_MODE_TEST or something as an argument for Bugzilla->usage_mode that will switch Bugzilla->localconfig to reading localconfig.test.

>ok (Bugzilla::Keyword::keyword_count() == 0 , "No keywords in DB");

  You should be using "is" instead of "ok" here. Or if you really want to force a numeric comparison, you should be using "cmp_ok". (Same comment for all your comparison tests.)

>my $keyword = Bugzilla::Keyword->create(
>    { name => 'foobar', description => 'Keyword foobar' });

  You should be calling "isa" on the return value, to make sure it returned something.

>$dbh->do("INSERT INTO keywords (bug_id, keywordid) VALUES (?,?)",
>            undef, 100, 1);
>
>$dbh->do("INSERT INTO keywords (bug_id, keywordid) VALUES (?,?)",
>            undef, 200, 1);

  There should be no direct SQL in any of your tests, because that will be hard to maintain.

>ok (scalar(@$allkeywords) == 1 && $allkeywords->[0]->{bug_count} == 2,
>    "Retrieved all keywords");

  You should never be accessing a hash member directly. Instead you should be calling the bug_count method.

  Also, that should be two separate tests, not one test.

>eval {
>    $keyword2 = Bugzilla::Keyword->create(
>        {name => 'foobar', description => 'Keyword foobar'});
>};

  Instead of calling an eval, you should use Test::Exception.

Comment 7

[Max Kanat-Alexander]

Also, please don't do work directly on this bug. This is a "meta bug" meaning that it tracks work to be done in other bugs. File a new bug specifically for the Keyword tests and make it block this one. (Make sure you CC me and LpSolit on the new bug, also.)

Comment 8

[Frédéric Buclin]

(In reply to comment #6)
> Instead of creating a DB manually and all that, I think that we should just
> have a separate localconfig.test or something, that checksetup.pl can create
> with a --test-db argument or something like that.

I would really prefer to separate the real DB and the test DB, to not mess the code with USAGE_MODE_TEST. We already have a test DB for Selenium; it would make more sense to use it.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> I would really prefer to separate the real DB and the test DB, to not mess the
> code with USAGE_MODE_TEST. We already have a test DB for Selenium; it would
> make more sense to use it.

  USAGE_MODE_TEST would only exist inside of Bugzilla->localconfig, and then it would only appear in the tests. That's not really messing the code.

Comment 10

[Max Kanat-Alexander]

Oh, but I do agree that we should re-use the same data that we currently use for the Selenium tests. We can import it incrementally into the main codebase. (I expect these Unit Tests to be a part of Bugzilla.)

Comment 11

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is either too invasive to be accepted for 4.4 at this point or shows no recent activity. The target milestone is reset and will be set again *only* when a patch is attached and approved.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else.