From: Simon.Cathebras Date: Fri, 08 Jun 2012 08:04:25 GMT Subject: Re: [PATCH 3/3] Tests file for git-remote-mediawiki Message-ID: <4FD1B209.702@ensimag.imag.fr> In-Reply-To: On 06/06/2012 22:18, Matthieu Moy wrote: > Simon Cathebras writes: > >> From: Charles Roussel >> >> 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. > >> 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 > >> +# 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. > Doesn't a directory containing 10 files pass the tests? > > You probably want a helper function test_contains_N_files 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. >> +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