Something weird here: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Koha/Object.t t/db_dependent/Koha/Object.t .. 3/11 # Failed test 'Exception thrown correctly' # at t/db_dependent/Koha/Object.t line 359. # expecting: Koha::Exceptions::Object::BadValue # found: DBIx::Class::Exception (DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: 'wrong_value' for column `koha_kohadev`.`borrowers`.`lastseen` at row 1 at /home/vagrant/kohaclone/Koha/Object.pm line 156 # ) # Looks like you failed 1 test of 1. # Failed test 'Bad value tests' # at t/db_dependent/Koha/Object.t line 363. # Looks like you failed 1 test of 16. Not on Jenkins It's the following exception's message that is not caught correctly by the regex: DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: 'wrong_value' for column `koha_kohadev`.`borrowers`.`lastseen` at row 1 at /home/vagrant/kohaclone/Koha/Object.pm line 156 Note the backticks.
FWIW I get the test passed with: - elsif( $_->{msg} =~ /Incorrect (?<type>\w+) value: '(?<value>.*)' for column '(?<property>\w+)'/ ) { + elsif( $_->{msg} =~ /Incorrect (?<type>\w+) value: '(?<value>.*)' for column (?<property>`\w+)/ ) { But I guess it will make it failed for someone else.
Created attachment 94401 [details] [review] Bug 23825: Koha/Object.t might fail on a backtick If the SQL error message contains a backtick instead of a regular quote, the regex for throwing an exception did not work. Example: Incorrect datetime value: 'wrong_value' for column `koha_master`.`borrowers`.`lastseen` Note the backtics where the regex contains a regular quote. This patch makes it more flexible: it allows one \W character before the column name, even optional. Test plan: Run Koha/Object.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Jonathan Druart from comment #1) > FWIW I get the test passed with: > > - elsif( $_->{msg} =~ /Incorrect (?<type>\w+) value: > '(?<value>.*)' for column '(?<property>\w+)'/ ) { > + elsif( $_->{msg} =~ /Incorrect (?<type>\w+) value: > '(?<value>.*)' for column (?<property>`\w+)/ ) { > > > But I guess it will make it failed for someone else. Yeah but we can make it a bit more flexible. See my patch. Could you QA it ?
Moving to SO: trivial change
Created attachment 94654 [details] [review] Bug 23825: Add tests
Marcel, I do not think it is correct. I have added some tests (they should not be there, I would have expected t/Koha/Exceptions.t to test that part). However I am not sure about the property. What was it before? table.column or only column? Anyway, now it's the DB name, which is obviously wrong.
Hmm Will have a look
(In reply to Jonathan Druart from comment #6) > Marcel, I do not think it is correct. I have added some tests (they should > not be there, I would have expected t/Koha/Exceptions.t to test that part). > However I am not sure about the property. What was it before? table.column > or only column? > Anyway, now it's the DB name, which is obviously wrong. You're right about the db name. But previously it did not work either. Apart from the quote, it also picked the first word (so the database). I will parse the whole thing into the property field. Hang on
Created attachment 94661 [details] [review] Bug 23825: Koha/Object.t might fail on a backtick If the SQL error message contains a backtick instead of a regular quote, the regex for throwing an exception did not work. Example: Incorrect datetime value: 'wrong_value' for column `koha_master`.`borrowers`.`lastseen` Note the backtics where the regex contains a regular quote. This patch makes it more flexible: it allows one \W character before the column name, even optional. Test plan: Run Koha/Object.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 94662 [details] [review] Bug 23825: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Changed 'is' to 'like' with regex since we also have the db name.
Do we know to root of the issue? Why do we have those tests failing suddenly?
(In reply to Jonathan Druart from comment #11) > Do we know to root of the issue? Why do we have those tests failing suddenly? Trigger for this report was the See also.
(In reply to Marcel de Rooy from comment #12) > (In reply to Jonathan Druart from comment #11) > > Do we know to root of the issue? Why do we have those tests failing suddenly? > > Trigger for this report was the See also. Yes, but it was not related. The issue existed before these patches were pushed.
(In reply to Jonathan Druart from comment #13) > (In reply to Marcel de Rooy from comment #12) > > (In reply to Jonathan Druart from comment #11) > > > Do we know to root of the issue? Why do we have those tests failing suddenly? > > > > Trigger for this report was the See also. > > Yes, but it was not related. The issue existed before these patches were > pushed. OK. There are two things: [1] Your second patch here introduces using the captured names of the regex like property. Which actually had the wrong value in the first place, but it was not used yet. So undetected. [2] On the see also report I noted that the regex in sub store did not work. Probably due to a difference between MySQL and MariaDB in using regular quotes or backtics.
(In reply to Marcel de Rooy from comment #14) > (In reply to Jonathan Druart from comment #13) > > (In reply to Marcel de Rooy from comment #12) > > > (In reply to Jonathan Druart from comment #11) > > > > Do we know to root of the issue? Why do we have those tests failing suddenly? > > > > > > Trigger for this report was the See also. > > > > Yes, but it was not related. The issue existed before these patches were > > pushed. > > OK. There are two things: > > [1] Your second patch here introduces using the captured names of the regex > like property. Which actually had the wrong value in the first place, but it > was not used yet. So undetected. > [2] On the see also report I noted that the regex in sub store did not work. > Probably due to a difference between MySQL and MariaDB in using regular > quotes or backtics. Marcel, I think we should not display the DB name in the message.
Created attachment 94803 [details] [review] Bug 23825: (QA follow-up) Remove database name As requested on comment15. Test plan: Run t/db_dependent/Koha/Object.t again
Created attachment 94804 [details] [review] Bug 23825: (QA follow-up) Remove database name As requested on comment15. Test plan: Run t/db_dependent/Koha/Object.t again Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
I'm having trouble reproducing this issue. Can you elaborate on the MySQL/MariaDB versions you are testing in?
(In reply to Tomás Cohen Arazi from comment #18) > I'm having trouble reproducing this issue. Can you elaborate on the > MySQL/MariaDB versions you are testing in? Thx for trying. mysql -v Server version: 10.4.4-MariaDB-1:10.4.4+maria~bionic mariadb.org binary distribution Running in a Docker container btw.
10.4.8-MariaDB-1:10.4.8+maria~stretch for me. Note that I upgraded using a specific repo (to get the latest), before (last week) I had the one packaged for Stretch and got the same failure.
I've now updated Jenkins to test a broader range of DB servers, but I've stuck to those that ship by default with our chosen supported OS's.. and as such we still won't be catching a DB server this new.. should we do you think?
(In reply to Martin Renvoize from comment #21) > I've now updated Jenkins to test a broader range of DB servers, but I've > stuck to those that ship by default with our chosen supported OS's.. and as > such we still won't be catching a DB server this new.. should we do you > think? Dont think so as to Jenkins personally. But this report actually only is about the type of quotes used in an error message, adds tests and improves the regex. So I do not really understand why this keeps hanging..
Downgrading from Major as this only affects very new DBMS versions which the majority of people will not be running. Having said that, so long as it doesn't break tests on the current stable DBMS versions then I don't have a problem with pushing it. I've not tested this yet myself, but if someone here can verify the tests continue to work with MySQL5.5, MariaDB10.1 and MariaDB10.3 then this feels OK to push at this point.
10.4.8-MariaDB-1:10.4.8+maria~stretch master: fail with patches: ok 10.1.41-MariaDB-0+deb9u1 master: ok with patches: fail # Failed test 'Column should be the expected one' # at t/db_dependent/Koha/Object.t line 413. # 'lastseen' # doesn't match '(?^u:borrowers\.lastseen)' # Looks like you failed 1 test of 3.
Created attachment 95044 [details] [review] Bug 23825: table name is not always displayed
With this last patch I got the tests passing for both 10.4.8-MariaDB-1:10.4.8+maria~stretch and 10.1.41-MariaDB-0+deb9u1
Created attachment 95053 [details] [review] Bug 23825: table name is not always displayed Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 95402 [details] [review] Bug 23825: Koha/Object.t might fail on a backtick If the SQL error message contains a backtick instead of a regular quote, the regex for throwing an exception did not work. Example: Incorrect datetime value: 'wrong_value' for column `koha_master`.`borrowers`.`lastseen` Note the backtics where the regex contains a regular quote. This patch makes it more flexible: it allows one \W character before the column name, even optional. Test plan: Run Koha/Object.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 95403 [details] [review] Bug 23825: Add tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] Changed 'is' to 'like' with regex since we also have the db name. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 95404 [details] [review] Bug 23825: (QA follow-up) Remove database name As requested on comment15. Test plan: Run t/db_dependent/Koha/Object.t again Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 95405 [details] [review] Bug 23825: table name is not always displayed Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for the perseverance here guys, Passing QA
Nice work! Pushed to master for 19.11.00
While testing the advertised "Experimental MariaDB 10.3 support" for 19.05.x, it turns out that this patch applied and fix two failure. Will try to backport for 19.11.13 For the record the fixed failures are: kohadev-koha@665e4031f5cf:/kohadevbox/koha$ time prove -r t/db_dependent/Circulation/MarkIssueReturned.t t/db_dependent/Koha/Object.t t/db_dependent/Circulation/MarkIssueReturned.t .. 2/4 DBD::mysql::st execute failed: Incorrect datetime value: 'bad_date' for column `koha_kohadev`.`issues`.`returndate` at row 1 [for Statement "UPDATE `issues` SET `returndate` = ? WHERE ( `issue_id` = ? )" with ParamValues: 0='bad_date', 1=252] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. # Failed test 'An exception is thrown on bad date' # at t/db_dependent/Circulation/MarkIssueReturned.t line 181. # expecting: Koha::Exceptions::Object::BadValue # found: DBIx::Class::Exception (DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: 'bad_date' for column `koha_kohadev`.`issues`.`returndate` at row 1 at /kohadevbox/koha/Koha/Object.pm line 157 # ) # Looks like you failed 1 test of 3. t/db_dependent/Circulation/MarkIssueReturned.t .. 3/4 # Failed test 'Manually pass a return date' # at t/db_dependent/Circulation/MarkIssueReturned.t line 185. t/db_dependent/Circulation/MarkIssueReturned.t .. 4/4 # Looks like you failed 1 test of 4. t/db_dependent/Circulation/MarkIssueReturned.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/4 subtests t/db_dependent/Koha/Object.t .................... 7/12 # Failed test 'Exception thrown correctly' # at t/db_dependent/Koha/Object.t line 359. # expecting: Koha::Exceptions::Object::BadValue # found: DBIx::Class::Exception (DBIx::Class::Storage::DBI::_dbh_execute(): Incorrect datetime value: 'wrong_value' for column `koha_kohadev`.`borrowers`.`lastseen` at row 1 at /kohadevbox/koha/Koha/Object.pm line 157 # ) # Looks like you failed 1 test of 1. # Failed test 'Bad value tests' # at t/db_dependent/Koha/Object.t line 363. # Looks like you failed 1 test of 16. t/db_dependent/Koha/Object.t .................... 10/12 # Failed test 'store() tests' # at t/db_dependent/Koha/Object.t line 366. # Looks like you failed 1 test of 12. t/db_dependent/Koha/Object.t .................... Dubious, test returned 1 (wstat 256, 0x100) Failed 1/12 subtests Test Summary Report ------------------- t/db_dependent/Circulation/MarkIssueReturned.t (Wstat: 256 Tests: 4 Failed: 1) Failed test: 3 Non-zero exit status: 1 t/db_dependent/Koha/Object.t (Wstat: 256 Tests: 12 Failed: 1) Failed test: 10 Non-zero exit status: 1 Files=2, Tests=16, 13 wallclock secs ( 0.05 usr 0.01 sys + 9.84 cusr 1.82 csys = 11.72 CPU) Result: FAIL
Good news: Backported to 19.05.x branch for 19.05.12 Thanks Tomas for the additional analysis :)