Re: [PATCH 2/3] Test environment of git-remote-mw
On 01/06/2012 13:49, Matthieu Moy wrote:
Show 20 quoted lines
> Guillaume Sasdy<guillaume.sasdy@ensimag.imag.fr> writes:
>
>> # CONFIGURATION VARIABLES
>> -# You might want to change those ones ...
>> +# You might want to change these ones
>> #
>> WIKI_DIR_NAME="wiki" # Name of the wiki's directory
>> WIKI_DIR_INST="/var/www" # Directory of the web server
>> TMP="/tmp" # Temporary directory for downloads
>> - # Absolute address needed!
>> + # Absolute path required!
>> SERVER_ADDR="localhost" # Web server's address
>>
>> -#
>> # CONFIGURATION
>> -# You should not change those ones unless you know what you to
>> +# You should not change these ones unless you know what you do
> I already mentionned in in your v1, but these fixups do not belong to
> PATCH 2/3. You do not want reviewers to see your mistakes in PATCH 1/2
> and see the fix in PATCH 2/3.
Got it. With the problem experienced yesterday, we had still this issue this morning. It is now fixed :).
Show 6 quoted lines
>> +wiki_getpage () {
>> + ../test-gitmw.pl get_page -p "$1" "$2"
>> +}
> Any reason why test-gitmw.pl and wiki_getpage have this slightly
> different API? The perl version has a "-p" flag, and the shell command
> has only positionnal arguments.The "-p" flag exists to specify if we have to use the admin login on wiki to do the command. For instance, here we fetch a page from the wiki with Admin privilege. Others arguments remains the same.
Show 6 quoted lines
> I'd rather have a more uniform way to wrap calls to test-gitmw.pl in
> shell, like
>
> wiki_<something> () {
> ../test-gitmw.pl<something> "$@"
> }Do you suggest we include the "-p" flag in <something> ? I agree for the use of "$@".
Show 5 quoted lines
>
> Then, you probably want to move the API documentation (i.e. comments you
> put before the shell functions) in, or next to the Perl script, and
> avoid repeating it in the shell.
>
Show 8 quoted lines
>> +# git_content<dir_1> <dir_2>
>> +#
>> +# Compare the contents of directories<dir_1> and<dir_2> with diff
>> +# and exits with error 1 if they do not match. The program will
>> +# not look into .git in the process.
>> +git_content () {
> Didn't I already say that the naming was strange? A function that
> compares something shouldn't be called just "content".Sorry, we misunderstood what you mean... Update is coming. What about git_diff_directories ? Because this shell function execute a special instance of diff, matching two directories and ignoring blank character.
Show 6 quoted lines
>> + result=$(diff -r -b --exclude=".git" "$1" "$2")
>> +
>> + if echo $result | grep -q ">" ; then
> Why grep, when the exit status of diff tells you about the differences
> already?
>
Good idea for the exit status.
Show 12 quoted lines
>> +# git_exist<rep_name> <file_name>
>> +#
>> +# Check the existence of<file_name> into the git repository
>> +#<rep_name> and exits with error 1 if it is absent.
>> +git_exist () {
>> + result=$(find "$1" -type f -name "$2")
>> +
>> + if ! echo $result | grep -q "$2" ; then
> Missing quotes around $result.
>
> Why do you need grep again? You just want to check whether "$result" is
> empty (test -z).this function is now erased from our code... Actually, it was really useless...
Show 13 quoted lines
>> + echo "test failed: file $2 does not exist in $1"
>> + exit 1
> die
>
> ?
>
>> +wiki_page_exist () {
>> + wiki_getpage "$1" .
>> +
>> + if test -f "$1".mw ; then
>> + echo "test failed: file $1 not found on wiki"
>> + exit 1
> likewise.We were supposed to do it with the previous version. But we are facing some difficulties using die function outside the test harness. We wanted to include the file "test-lib.sh" in our own script "test-gitmw-lib.sh" to use die. It doesn't work, because the harness expect the file "test-gitmw-lib.sh" to be a test script. Wich is not the case.
Do you have any Idea about how to fix this problem ?
Show 5 quoted lines
>> +fail()
>> +{
> Style: fail () {
>
> Plus, didn't we say "die" was already there for this?Show 9 quoted lines
>> + # Replace spaces by underscore in the page name
>> + $pagename=~s/\ /_/;
> Indent with space (didn't I already mention that?).
>
>> +my $login = $ARGV[0];
>> +
>> +if ($login eq "-p")
> Feels weird. If you're not sure it's a login, why call the variable
> $login?
Change on the way. renaming this variable "is_login". Is this ok for you ?
> Any reason not to use Perl's option parsing?
Not at all, we juste didn't thought about it.
--
CATHEBRAS Simon
2A-ENSIMAG
Filière Ingéniérie des Systèmes d'Information
Membre Bug-Buster
Simon.Cathebras