While working on Bug 15870, koha qa test tools gave this error fatal: ambiguous argument 't/db_dependent/RecordProcessor_ViewPolicy.t': unknown revision or path not in the working tree. This is in part, because a test file was moved/renamed and the files changed list includes the old filename. Gracefully handle the file no longer existing.
Created attachment 48830 [details] [review] Bug 16008: noisy "fatal" warning when new file is moved in another patch The failure was tracked down to check_forbidden_patterns in QohA/File.pm's check_forbidden_patterns function. As there can't be any forbidden patterns on a non-existent file, this patch returns the default 1 when the file being checked does not exist. TEST PLAN --------- apply bug 15870 to your koha run qa test tools -- see the warning apply this patch to your qa test tools run qa test tools -- no more warning
Hi Mark, Actually it's a ... feature :) You are not supposed to add a file and remove it in the same patch set. It should be a new check actually, but at the moment I would prefer to keep the warning.
Created attachment 48862 [details] [review] Bug 16008: noisy "fatal" warning when new file is moved in another patch The failure was tracked down to check_forbidden_patterns in QohA/File.pm's check_forbidden_patterns function. As there can't be any forbidden patterns on a non-existent file, this patch returns the default 1 when the file being checked does not exist. It maintains a warning message as suggested in comment #2. It is better to create another check as stated in comment #2, but I figure I'll share my less than perfect work anyways. TEST PLAN --------- apply bug 15870 to your koha run qa test tools -- see the warning apply this patch to your qa test tools run qa test tools -- no more warning
Mark, The patch works but it's not the proper way to handle this error. Other checks just return 0 (and not 1) if the file does not exist. It does not make sense to test this failure in the check_forbidden_patterns subroutine.
Created attachment 49318 [details] [review] [COUNTER-PATCH] Bug 16008: Deal with removed files Bug 16008: Deal with removed files This patch fixes 2 issues: 1/ When a patch just removes files, the $git->log method did not return the right list of files. The commit which only removed files were not counted. 2/ If a file is added then deleted in the same patch set, the following error occurred: fatal: ambiguous argument 't/db_dependent/RecordProcessor_ViewPolicy.t': unknown revision or path not in the working tree. Now it's correctly handled: a new 'git manipulation' check has been added. Note that lot of changes has been made to achieve this goal: 1/ QohA::Files->filter has been removed: it was not needed the assignement was done in the BUILDER 2/ QohA::Git->log now returns all changes (even for deleted files) 3/ QohA::Report->type has been deleted, it was not used 4/ The report member initialization has been moved to the parent class (QohA::File). Test plan: % echo "#simple file" > new_file % echo "#comment" >> mainpage.pl % git add new_file mainpage.pl; git commit -m'1' % git rm new_file % git commit -m'2' % qa -c 2 -v 2 # should return a warning for new_file
You will need to update the koha-qa repo and apply 16104 first to make the tests pass.
Mark, any chances to see you testing this patch?
(In reply to Jonathan Druart from comment #7) > Mark, any chances to see you testing this patch? I'll let QA test this harder, but I have tested for the newer git manipulation test which you added (nice simplification) and forbidden patterns (since I hit that with 16236 last night). Spelling, tt_valid, and valid_template failures should still be confirmed.
Created attachment 50136 [details] [review] [SIGNED-OFF] Bug 16008: Deal with removed files This patch fixes 2 issues: 1/ When a patch just removes files, the $git->log method did not return the right list of files. The commit which only removed files were not counted. 2/ If a file is added then deleted in the same patch set, the following error occurred: fatal: ambiguous argument 't/db_dependent/RecordProcessor_ViewPolicy.t': unknown revision or path not in the working tree. Now it's correctly handled: a new 'git manipulation' check has been added. Note that lot of changes has been made to achieve this goal: 1/ QohA::Files->filter has been removed: it was not needed the assignement was done in the BUILDER 2/ QohA::Git->log now returns all changes (even for deleted files) 3/ QohA::Report->type has been deleted, it was not used 4/ The report member initialization has been moved to the parent class (QohA::File). Test plan: % echo "#simple file" > new_file % echo "#comment" >> mainpage.pl % git add new_file mainpage.pl; git commit -m'1' % git rm new_file % git commit -m'2' % qa -c 2 -v 2 # should return a warning for new_file NOTE: ran this test plan, and checked forbidden patterns. need confirmation on spelling, tt_valid, and valid_template. Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Thanks Mark!
Too bad not to see another QAer taking a look at this one. I am going to push it anyway.
(In reply to Jonathan Druart from comment #11) > Too bad not to see another QAer taking a look at this one. > I am going to push it anyway. No objection at all for doing so. If there would be an error, you will hear :) Fetched latest commit now.. ;)
Now tested again with Bug 15839 Error is gone, but I see a few 0's and 1's (debug stuff?) OK C4/Review.pm OK critic OK forbidden patterns 0 OK git manipulation OK pod SKIP spelling OK valid 0 FAIL Koha/Review.pm OK critic OK forbidden patterns 1
(In reply to Marcel de Rooy from comment #13) > Now tested again with Bug 15839 > Error is gone, but I see a few 0's and 1's (debug stuff?) > > OK C4/Review.pm > OK critic > OK forbidden patterns > 0 > OK git manipulation > OK pod > SKIP spelling > OK valid > 0 > > FAIL Koha/Review.pm > OK critic > OK forbidden patterns > 1 Yes I have seen it too, but I think it already existed prior to this patch.
Marcel, commit 687b5cd790bbb0378a91d1896bcd947bbe76bec1 Display the errors if it's a valid error Will fix the 0 and 1