Bug 16008 - noisy "fatal" warning when new file is moved in another patch
Summary: noisy "fatal" warning when new file is moved in another patch
Status: CLOSED FIXED
Alias: None
Product: Project Infrastructure
Classification: Unclassified
Component: QA tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement
Assignee: Jonathan Druart
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-08 21:31 UTC by Mark Tompsett
Modified: 2017-12-07 22:15 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 16008: noisy "fatal" warning when new file is moved in another patch (1.17 KB, patch)
2016-03-08 21:37 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16008: noisy "fatal" warning when new file is moved in another patch (1.50 KB, patch)
2016-03-09 14:11 UTC, Mark Tompsett
Details | Diff | Splinter Review
[COUNTER-PATCH] Bug 16008: Deal with removed files (19.36 KB, patch)
2016-03-18 14:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16008: Deal with removed files (19.54 KB, patch)
2016-04-12 00:01 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2016-03-08 21:31:23 UTC
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.
Comment 1 Mark Tompsett 2016-03-08 21:37:01 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-03-09 09:32:16 UTC
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.
Comment 3 Mark Tompsett 2016-03-09 14:11:34 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-03-18 10:43:41 UTC
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.
Comment 5 Jonathan Druart 2016-03-18 14:31:31 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-03-18 14:33:50 UTC
You will need to update the koha-qa repo and apply 16104 first to make the tests pass.
Comment 7 Jonathan Druart 2016-04-11 11:17:46 UTC
Mark, any chances to see you testing this patch?
Comment 8 Mark Tompsett 2016-04-11 23:58:33 UTC
(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.
Comment 9 Mark Tompsett 2016-04-12 00:01:10 UTC
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>
Comment 10 Jonathan Druart 2016-04-12 08:06:29 UTC
Thanks Mark!
Comment 11 Jonathan Druart 2016-06-10 14:29:09 UTC
Too bad not to see another QAer taking a look at this one.
I am going to push it anyway.
Comment 12 Marcel de Rooy 2016-07-22 09:34:38 UTC
(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.. ;)
Comment 13 Marcel de Rooy 2016-07-22 09:37:45 UTC
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
Comment 14 Jonathan Druart 2016-07-22 09:45:10 UTC
(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.
Comment 15 Jonathan Druart 2016-07-22 10:40:51 UTC
Marcel,
  commit 687b5cd790bbb0378a91d1896bcd947bbe76bec1
    Display the errors if it's a valid error
Will fix the 0 and 1