Re: [PATCH 3/3] Tests file for git-remote-mediawiki

From: Simon.Cathebras <simon.cathebras@ensimag.imag.fr>
Date: Jun 8, 2012, 08:04 UTC
Message-ID: <4FD1B209.702@ensimag.imag.fr>
In-Reply-To: <vpqobow8a9l.fsf@bauges.imag.fr>

On 06/06/2012 22:18, Matthieu Moy wrote:

Show 7 quoted lines

> Simon Cathebras<simon.cathebras@ensimag.imag.fr>  writes:
>
>> From: Charles Roussel<charles.roussel@ensimag.fr>
>>
>> Those scripts test the functions of git-remote-mediawiki.
>> t9360: test cases for git clone. Including tests of cloning just a category, just a precise set of page and a classical use of clone on the whole wiki.
> Avoid long lines in code and commit messages (80 columns max)

Understood, it will be corrected in the next patch.

Show 9 quoted lines

>
>> In addition, this file provide now some fonction du manipulate sections on wiki.
> s/du/to/
>
>> t9361: test cases for git pull (add page, edit page, delete page) and git push (add file, edit file, delete file).
> When commis messages start looking like an enumeration, it usually means
> that either you read the GNU recommandation for ChangeLogs too much, or
> that you should split your commit (not mandatory here I think, but short
> patches are easier to review).

Do you mean that we should split the third patch into two patches ? For 
instance::
     Patch 3/4: tests for git pull
     Patch 4/4: tests for git push

Show 5 quoted lines

>
>> +# tests for git-remote-mediawiki
>> +
>> +test_description='Test the Git Mediawiki remote helper: git clone'
> Why do you need a comment if you have the test_description right below?

Deleted.

>
>> +if [ ! -f /$GIT_BUILD_DIR/git-remote-mediawiki ];
> Why / in front of $GIT_BUILD_DIR/ ?

Good point, we didn't noticed it ...

>> +        test_expect_code 0 ls mw_dir | wc -l | grep 1&&
>> +        test_expect_code 0 test -e mw_dir/Main_Page.mw&&
> Why "test_expect_code 0"? You already have&&  right?

Yes, we haven't realised it wasn't necessary. We have corrected other errors like this one in our tests.

Show 5 quoted lines

> Doesn't a directory containing 10 files pass the tests?
>
> You probably want a helper function test_contains_N_files<dir>  <N>  that
> does test `ls mw_dir | wc -l` -eq 1 as you did below, but may give a
> diagnosis when the test fails.

Done, and replace within the existing code.

Show 6 quoted lines

>> +test_expect_success 'git clone only create Main_Page.mw with a wiki with no other pages ' '
>> +        wiki_reset&&
>> +        wiki_editpage foo "this page must be delete before the clone" false&&
> s/delete/deleted/
>> +	git_diff_directories mw_dir ref_page&&
> functions in tests are usually prefixed with test_ instead.

Fair enough. We have changed the function's name. Should we add such prefix on functions like wiki_reset or wiki_delete page ?

>> +# clone a wiki after a page has been added then edited once
>> +# check that the content is correct
> It's not sufficient. You should check also that the history is correct.
>

Test is now correct.

Thanks for the advices ;).

-- 
CATHEBRAS Simon

2A-ENSIMAG

Filière Ingéniérie des Systèmes d'Information
Membre Bug-Buster