Bug 20767 - "The method is not covered by tests!" should give more information
Summary: "The method is not covered by tests!" should give more information
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-05-14 17:03 UTC by Jonathan Druart
Modified: 2018-06-06 12:05 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20767: Display a stack trace when a method is not covered by tests (5.08 KB, patch)
2018-05-14 17:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20767: Display a stack trace when a method is not covered by tests (5.13 KB, patch)
2018-05-24 16:08 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 20767: Display a stack trace when a method is not covered by tests (5.21 KB, patch)
2018-05-25 08:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-05-14 17:03:56 UTC
When a method is not allowed yet (not covered by tests), developers get an error the is not really useful: "The method XXX is not covered by tests!"
It would be more useful to have a stack trace in order to know where this error come from.
Comment 1 Jonathan Druart 2018-05-14 17:05:18 UTC
Created attachment 75319 [details] [review]
Bug 20767: Display a stack trace when a method is not covered by tests

When a method is not allowed yet (not covered by tests), developers get
an error the is not really useful: "The method XXX is not covered by
tests!"
It would be more useful to have a stack trace in order to know where
this error come from.

Test plan:
Modify some code to display this stack trace and confirm it makes sense
to have it, i.e. it will make debug easier
Comment 2 Victor Grousset/tuxayo 2018-05-17 16:38:12 UTC
(In reply to Jonathan Druart from comment #0)
> When a method is not allowed yet (not covered by tests)

Is this about automated tests? Then what does this has to do when executing the method? Is this a way to make people to write tests?

It seems that there can be other causes to this error, is that correct?

I also heard that this had to do with the mapping with de DB columns being done or not. So, I'm confused ^^"
Comment 3 Julian Maurice 2018-05-18 07:06:30 UTC
(In reply to Victor Grousset/tuxayo from comment #2)
> (In reply to Jonathan Druart from comment #0)
> > When a method is not allowed yet (not covered by tests)
> 
> Is this about automated tests? Then what does this has to do when executing
> the method? Is this a way to make people to write tests?
> 
> It seems that there can be other causes to this error, is that correct?
> 
> I also heard that this had to do with the mapping with de DB columns being
> done or not. So, I'm confused ^^"

We can get this error when we try to use a new column and Koha::Schema was not updated.
So, the message should probably be "This method does not exist" rather than "not covered by tests".
Comment 4 Séverine Queune 2018-05-24 13:03:00 UTC
I have a (good ?) example on my test instance : when I try to use the new syspref to mark an item lost and notify patron from holds to pull list, I get this software error :
    The method homebranch is not covered by tests!"

Applying this patch, I get this one :

    The method Koha::Patron->homebranch is not covered by tests!

    Trace begun at /var/repositories/koha/Koha/Object.pm line 398
    Koha::Object::AUTOLOAD('Koha::Patron=HASH(0x8d10e08)') called at /var/repositories/koha/
    circ/pendingreserves.pl line 71

Is it the behaviour you were looking for ?
As I don't know what you expect I prefer not signing it.
Comment 5 M. Tompsett 2018-05-24 16:08:10 UTC
Created attachment 75547 [details] [review]
Bug 20767: Display a stack trace when a method is not covered by tests

When a method is not allowed yet (not covered by tests), developers get
an error the is not really useful: "The method XXX is not covered by
tests!"
It would be more useful to have a stack trace in order to know where
this error come from.

Test plan:
Modify some code to display this stack trace and confirm it makes sense
to have it, i.e. it will make debug easier

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 6 Marcel de Rooy 2018-05-25 08:06:29 UTC
Created attachment 75552 [details] [review]
Bug 20767: Display a stack trace when a method is not covered by tests

When a method is not allowed yet (not covered by tests), developers get
an error the is not really useful: "The method XXX is not covered by
tests!"
It would be more useful to have a stack trace in order to know where
this error come from.

Test plan:
Modify some code to display this stack trace and confirm it makes sense
to have it, i.e. it will make debug easier

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Julian Maurice 2018-05-25 08:09:34 UTC
(In reply to Julian Maurice from comment #3)
> So, the message should probably be "This method does not exist" rather than
> "not covered by tests".

Any opinion on that ? Should I open a new bug ?
Comment 8 Marcel de Rooy 2018-05-25 09:01:47 UTC
(In reply to Julian Maurice from comment #7)
> (In reply to Julian Maurice from comment #3)
> > So, the message should probably be "This method does not exist" rather than
> > "not covered by tests".
> 
> Any opinion on that ? Should I open a new bug ?

This method is not supported ? If you only replace a few words, please add a QA follow-up?
Comment 9 Tomás Cohen Arazi 2018-05-25 11:41:02 UTC
(In reply to Julian Maurice from comment #7)
> (In reply to Julian Maurice from comment #3)
> > So, the message should probably be "This method does not exist" rather than
> > "not covered by tests".
> 
> Any opinion on that ? Should I open a new bug ?

I agree. File a new one!
Comment 10 Nick Clemens 2018-05-29 14:38:37 UTC
Pushed to master for 18.11.

Awesome work all, thank you.
Comment 11 Jonathan Druart 2018-06-01 16:58:50 UTC
We need it to be backported to help debug!
Comment 12 Martin Renvoize 2018-06-05 10:25:24 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01

Go forth and debug all the things :)
Comment 13 Fridolin SOMERS 2018-06-06 12:01:32 UTC
Pushed to 17.11.x for 17.11.07
Comment 14 Fridolin SOMERS 2018-06-06 12:05:55 UTC
I choose not to backport to 17.05.x, I focus on major and blocker