Bug 22001 - Add RaiseError and PrintError flags for all tests
Summary: Add RaiseError and PrintError flags for all tests
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 14334
Blocks: 25018 25026
  Show dependency treegraph
 
Reported: 2018-12-14 16:08 UTC by Jonathan Druart
Modified: 2020-05-20 09:57 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 22001: Remove the RaiseError occurrences from tests (17.99 KB, patch)
2018-12-14 16:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Add tests (939 bytes, patch)
2018-12-14 16:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Enable RaiseError for tests (1.38 KB, patch)
2018-12-14 16:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Remove the RaiseError occurrences from tests (19.86 KB, patch)
2020-02-14 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Add tests (939 bytes, patch)
2020-02-14 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Enable RaiseError for tests (1.38 KB, patch)
2020-02-14 09:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Remove the RaiseError occurrences from tests (19.92 KB, patch)
2020-02-25 14:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22001: Add tests (1004 bytes, patch)
2020-02-25 14:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22001: Enable RaiseError for tests (1.44 KB, patch)
2020-02-25 14:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22002: (RM follow-up) Allow RaiseError tests in Reports/Guided.t (2.69 KB, patch)
2020-03-27 11:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22001: (RM follow-up) Allow RaiseError tests in Reports/Guided.t (3.32 KB, patch)
2020-03-27 11:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22001: Fix Reports/Guided.t tests (4.36 KB, patch)
2020-03-27 11:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Fix Object.t - handle correct RaiseError and PrintError (4.23 KB, patch)
2020-03-30 09:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Set unsafe only if RaiseError is not set (2.28 KB, patch)
2020-03-31 14:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Add the DBI error message (925 bytes, patch)
2020-03-31 15:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Make the DBD error regex less strict (7.18 KB, patch)
2020-03-31 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Fix Koha/Patrons.t (1.38 KB, patch)
2020-03-31 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: execute reports in an eval (1.69 KB, patch)
2020-03-31 15:59 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22001: Errors are no longer logged by default (!) (2.29 KB, patch)
2020-03-31 15:59 UTC, Jonathan Druart
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-12-14 16:08:46 UTC
Bug 14334 removed some of the RaiseError flags that existed in tests.

It should always be set in tests, just in case :)

Moreover, patches of bug 14334 could hide a regression, if we have a test catching a crash (using eval), it will no longer explode.
Comment 1 Jonathan Druart 2018-12-14 16:31:52 UTC
Created attachment 83230 [details] [review]
Bug 22001: Remove the RaiseError occurrences from tests

Unless it is needed!
Also remove $dbh when not used later.
Comment 2 Jonathan Druart 2018-12-14 16:31:55 UTC
Created attachment 83231 [details] [review]
Bug 22001: Add tests
Comment 3 Jonathan Druart 2018-12-14 16:31:59 UTC
Created attachment 83232 [details] [review]
Bug 22001: Enable RaiseError for tests
Comment 4 Jonathan Druart 2020-02-14 09:00:25 UTC
Created attachment 98910 [details] [review]
Bug 22001: Remove the RaiseError occurrences from tests

Unless it is needed!
Also remove $dbh when not used later.
Comment 5 Jonathan Druart 2020-02-14 09:00:29 UTC
Created attachment 98911 [details] [review]
Bug 22001: Add tests
Comment 6 Jonathan Druart 2020-02-14 09:00:33 UTC
Created attachment 98912 [details] [review]
Bug 22001: Enable RaiseError for tests
Comment 7 Martin Renvoize 2020-02-25 14:46:08 UTC
Created attachment 99589 [details] [review]
Bug 22001: Remove the RaiseError occurrences from tests

Unless it is needed!
Also remove $dbh when not used later.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-02-25 14:46:11 UTC
Created attachment 99590 [details] [review]
Bug 22001: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-02-25 14:46:14 UTC
Created attachment 99591 [details] [review]
Bug 22001: Enable RaiseError for tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-02-25 14:50:15 UTC
Works as expected.. sorry I didn't spot this one earlier :(
Comment 11 Martin Renvoize 2020-03-26 15:10:09 UTC
I'm happy with this one.. it's important for us to see the errors being thrown.. Passing QA
Comment 12 Martin Renvoize 2020-03-27 08:53:27 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 13 Martin Renvoize 2020-03-27 11:14:21 UTC
Created attachment 101989 [details] [review]
Bug 22002: (RM follow-up) Allow RaiseError tests in Reports/Guided.t
Comment 14 Martin Renvoize 2020-03-27 11:32:04 UTC
Created attachment 101990 [details] [review]
Bug 22001: (RM follow-up) Allow RaiseError tests in Reports/Guided.t
Comment 15 Martin Renvoize 2020-03-27 11:32:30 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 16 Jonathan Druart 2020-03-27 11:34:01 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2020-03-30 09:06:13 UTC
Created attachment 102070 [details] [review]
Bug 22001: Fix Object.t - handle correct RaiseError and PrintError
Comment 18 Martin Renvoize 2020-03-31 08:51:28 UTC
Final followup pushed.
Comment 19 Jonathan Druart 2020-03-31 14:07:50 UTC
Created attachment 102140 [details] [review]
Bug 22001: Set unsafe only if RaiseError is not set

Sooooo....

That was tricky, and the solution looks trivial.
However it's not.
We have unsafe set for "historical reason".
Having it on when RaiseError is on have the effect of overwritting the
DBIC error handler.

The problem is:
t/db_dependent/Circulation/MarkIssueReturned.t (and other tests) is failing with:

  # expecting: Koha::Exceptions::Object::BadValue
  # found: DBIx::Class::Exception ({UNKNOWN}: Can't locate object method "rethrow" via package "DBD::mysql::st execute failed: Incorrect datetime value: 'bad_date' for column 'returndate' at row 1 [for Statement "UPDATE `issues` SET `returndate` = ? WHERE ( `issue_id` = ? )" with ParamValues: 0='bad_date', 1=238] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line

In Koha::Object->store, the exception is not a DBIx::Class::Exception object (as we except), but
a string (on which we cannot call rethrow).

Swithing unsafe off restores the expected behavior.
To make sure the UI will not be affected, it is only turned off when
RaiseError is set.

The situation is still wrong (for UI), from the POD https://metacpan.org/pod/DBIx::Class::Storage::DBI (/unsafe)
"""
Note that your custom settings can cause Storage to malfunction, especially if you set a HandleError handler that suppresses exceptions and/or disable RaiseError.
"""

And also https://metacpan.org/release/DBIx-Class/source/lib/DBIx/Class/Storage/DBI.pm#L1531

Many thanks Tomas for the digging exploration!

We need to turn RaiseError and remove the unsafe flag, for UI as well,
but that should be done at the beginning of a dev cycle.
Comment 20 Martin Renvoize 2020-03-31 14:47:36 UTC
Final followup pushed.
Comment 21 Jonathan Druart 2020-03-31 15:28:20 UTC
Created attachment 102147 [details] [review]
Bug 22001: Add the DBI error message

Using DBIx::Class error handler, we should not hide the error when
encapsulating it
Comment 22 Jonathan Druart 2020-03-31 15:59:04 UTC
Created attachment 102149 [details] [review]
Bug 22001: Make the DBD error regex less strict

Now the message is looking like
DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Duplicate entry
Comment 23 Jonathan Druart 2020-03-31 15:59:08 UTC
Created attachment 102150 [details] [review]
Bug 22001: Fix Koha/Patrons.t

The message is only printed once
Comment 24 Jonathan Druart 2020-03-31 15:59:13 UTC
Created attachment 102151 [details] [review]
Bug 22001: execute reports in an eval

Otherwise the tests will fail. We will certainly log twice the error
when run from the UI, but not a big deal. This definitely needs more
attention in a follow-up bug report.
We want to raise proper exceptions here.
Comment 25 Jonathan Druart 2020-03-31 15:59:16 UTC
Created attachment 102152 [details] [review]
Bug 22001: Errors are no longer logged by default (!)

This is one of the biggest part we will have to deal with when we will
switch to DBIC handler for UI as well.
The DBI errors will not be logged if we do not deal with them correctly.
Comment 26 Martin Renvoize 2020-03-31 16:51:05 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Joy Nelson 2020-04-03 22:47:19 UTC
enhancement not backported to 19.11.x