Created attachment 109599 [details] [review] Bug 26364: Check that return value isn't the value we don't want. Don't check for undef It seems sometimes we get an ISBN returned that matches a book in the sample data. One of the Harry Potter books was matching the ISBN. Our real concern is not that we get nothing, but that we don't get the same value we pass in. We should test for that To test: 1 - prove -v t/db_dependent/XISBN.t 2 - The failures are not consistent, so read the code and ensure we are making sense Can you explain? Is it an alternative patch for bug 26270? Where did you see it fail? (In reply to Jonathan Druart from comment #3) > Where did you see it fail? It was failing on our package builds, but not locally It seemed that the ISBN previously returned nothing, but now had an ISBN that matched one in our system The change makes sense though, we just need to confirm we don't get the biblionumber, not that we get nothing ThingISBN can change and makes no guarantees, so our tests must be explicit (In reply to Nick Clemens from comment #4) > (In reply to Jonathan Druart from comment #3) > > Where did you see it fail? > > It was failing on our package builds, but not locally > > It seemed that the ISBN previously returned nothing, but now had an ISBN > that matched one in our system > > The change makes sense though, we just need to confirm we don't get the > biblionumber, not that we get nothing > > ThingISBN can change and makes no guarantees, so our tests must be explicit bug 26270 skips if the normalized ISBNs are equal to the xisbns it should only do so if the biblionumber is the same - isbn_bib1 = 0590353403 isbn_bib2 = 0684843897 isbn_bib3 = 043936213X i.e. we send 043936213X, thing ISBN returns 0590353403 with 26270 we skip the results because normalized 0590353403 = 0590353403 but that is ok the test before failed because it said 043936213X returned nothing ThingISBN now returns 0590353403 for 043936213X so we get bib1 - as long as we don't get bib3 then things are working Created attachment 109631 [details] [review] Bug 26364: Check that return value isn't the value we don't want. Don't check for undef It seems sometimes we get an ISBN returned that matches a book in the sample data. One of the Harry Potter books was matching the ISBN. Our real concern is not that we get nothing, but that we don't get the same value we pass in. We should test for that To test: 1 - prove -v t/db_dependent/XISBN.t 2 - The failures are not consistent, so read the code and ensure we are making sense > 2 - The failures are not consistent, so read the code and ensure we are > making sense No, the failure is consistent if you don't have bug 26270. Nick, did you understand why this test start failing? I guess the service changes their return value. Do you know what has changed? Created attachment 109821 [details] [review] Bug 26364: Don't skip records that match the isbn we passed Created attachment 109822 [details] [review] Bug 26364: Remove false matches on isbn The test here was returning 0590353403 when searched for 9780590353403 because of the regex matching Koha doesn't work this way unless SearchWithISBNVariations is set to 'search' and you use a dropdown to select ISBN for searching To test: 1 - Search catalog for nb=9780590353403 2 - Confirm you don't have results, or delete the records with results 3 - Add isbn 0590353403 to a record 4 - Repeat search, fails 5 - Enable SearchWithISBNVariations 6 - Repeat search, fails 7 - Go to advanced search, select ISBN, search for 9780590353403 8 - Get results Created attachment 109823 [details] [review] Bug 26364: Remove useless condition $response_data is a hash with one key, content Why are we checkiing it against a string? Created attachment 109824 [details] [review] Bug 26364: Don't discard the passed isbn I don't know, if we get rid fo this check we can now check the db for the same isbn we passed This may be useful as we may want to see the biblio with the same isbn as long as number is different? This will make tests fail Created attachment 109825 [details] [review] Bug 26364: Adjust unit tests We shouldn't rely on knowing exactly which ISBNs we get, we should specifically check for what we do or don't expect. I believe we should return biblios that have the same isbn we passed, as it signlas we have another biblio in the catalog that matches the one we are on. To test: prove -v t/db_dependent/XISBN.t To test: 1 - Apply patch 1 2 - prove -v t/db_dependent/XISBN.t 3 - test fails 4 - Apply patch 2 5 - tests pass 6 - Apply patch 3 - no change 7 - Apply patch 4 - tests fail 8 - Apply patch 5 - tests pass Created attachment 113054 [details] [review] Bug 26364: Don't skip records that match the isbn we passed Signed-off-by: David Nind <david@davidnind.com> Created attachment 113055 [details] [review] Bug 26364: Remove false matches on isbn The test here was returning 0590353403 when searched for 9780590353403 because of the regex matching Koha doesn't work this way unless SearchWithISBNVariations is set to 'search' and you use a dropdown to select ISBN for searching To test: 1 - Search catalog for nb=9780590353403 2 - Confirm you don't have results, or delete the records with results 3 - Add isbn 0590353403 to a record 4 - Repeat search, fails 5 - Enable SearchWithISBNVariations 6 - Repeat search, fails 7 - Go to advanced search, select ISBN, search for 9780590353403 8 - Get results Signed-off-by: David Nind <david@davidnind.com> Created attachment 113056 [details] [review] Bug 26364: Remove useless condition $response_data is a hash with one key, content Why are we checkiing it against a string? Signed-off-by: David Nind <david@davidnind.com> Created attachment 113057 [details] [review] Bug 26364: Don't discard the passed isbn I don't know, if we get rid fo this check we can now check the db for the same isbn we passed This may be useful as we may want to see the biblio with the same isbn as long as number is different? This will make tests fail Signed-off-by: David Nind <david@davidnind.com> Created attachment 113058 [details] [review] Bug 26364: Adjust unit tests We shouldn't rely on knowing exactly which ISBNs we get, we should specifically check for what we do or don't expect. I believe we should return biblios that have the same isbn we passed, as it signlas we have another biblio in the catalog that matches the one we are on. To test: prove -v t/db_dependent/XISBN.t Signed-off-by: David Nind <david@davidnind.com> To test, I followed the test plans in comment #13, #15 and #18. Created attachment 114818 [details] [review] Bug 26364: Don't skip records that match the isbn we passed Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 114819 [details] [review] Bug 26364: Remove false matches on isbn The test here was returning 0590353403 when searched for 9780590353403 because of the regex matching Koha doesn't work this way unless SearchWithISBNVariations is set to 'search' and you use a dropdown to select ISBN for searching To test: 1 - Search catalog for nb=9780590353403 2 - Confirm you don't have results, or delete the records with results 3 - Add isbn 0590353403 to a record 4 - Repeat search, fails 5 - Enable SearchWithISBNVariations 6 - Repeat search, fails 7 - Go to advanced search, select ISBN, search for 9780590353403 8 - Get results Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 114820 [details] [review] Bug 26364: Remove useless condition $response_data is a hash with one key, content Why are we checkiing it against a string? Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 114821 [details] [review] Bug 26364: Don't discard the passed isbn I don't know, if we get rid fo this check we can now check the db for the same isbn we passed This may be useful as we may want to see the biblio with the same isbn as long as number is different? This will make tests fail Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 114822 [details] [review] Bug 26364: Adjust unit tests We shouldn't rely on knowing exactly which ISBNs we get, we should specifically check for what we do or don't expect. I believe we should return biblios that have the same isbn we passed, as it signlas we have another biblio in the catalog that matches the one we are on. To test: prove -v t/db_dependent/XISBN.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> (In reply to Jonathan Druart from comment #7) > > 2 - The failures are not consistent, so read the code and ensure we are > > making sense > > No, the failure is consistent if you don't have bug 26270. > > Nick, did you understand why this test start failing? I guess the service > changes their return value. Do you know what has changed? I am not sure if this question was answered on another channel. Nick, can you please comment? Patches apply and QA tests pass. Unit tests as well. (In reply to Katrin Fischer from comment #25) > (In reply to Jonathan Druart from comment #7) > > Nick, did you understand why this test start failing? I guess the service > > changes their return value. Do you know what has changed? > > I am not sure if this question was answered on another channel. Nick, can > you please comment? comment #5: ThingISBN now returns 0590353403 for 043936213X so we get a match we didn't get previously Pushed to master for 21.05, thanks to everybody involved! Pushed to 20.11.x for 20.11.02 Pushed to 20.05.x for 20.05.08 Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed. |
75 eval { $results_thingisbn = C4::XISBN::get_xisbns($isbn1,$biblionumber3); }; 76 SKIP: { 77 skip "Problem retrieving ThingISBN", 1 78 unless $@ eq ''; 79 is( $results_thingisbn->[0]->{biblionumber}, 80 undef, 81 "Doesn't get biblionumber if the biblionumber matches the one passed to the sub." ); 82 } Here we are testing that we don't get the same book that we passed into the sub in the list that is returned, however, we shouldn't assume that we get nothing, we should just check that it is not the biblionumber we passed in