Bug 26364 - XISBN.t makes a bad assumption about return values
Summary: XISBN.t makes a bad assumption about return values
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on: 26270
Blocks: 28288
  Show dependency treegraph
 
Reported: 2020-09-03 09:53 UTC by Nick Clemens
Modified: 2021-05-04 14:38 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.02,20.05.08


Attachments
Bug 26364: Check that return value isn't the value we don't want. Don't check for undef (1.28 KB, patch)
2020-09-03 09:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26364: Check that return value isn't the value we don't want. Don't check for undef (1.82 KB, patch)
2020-09-03 15:47 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26364: Don't skip records that match the isbn we passed (815 bytes, patch)
2020-09-09 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26364: Remove false matches on isbn (1.47 KB, patch)
2020-09-09 14:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26364: Remove useless condition (925 bytes, patch)
2020-09-09 14:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26364: Don't discard the passed isbn (1.00 KB, patch)
2020-09-09 14:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26364: Adjust unit tests (2.64 KB, patch)
2020-09-09 14:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26364: Don't skip records that match the isbn we passed (863 bytes, patch)
2020-11-05 09:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 26364: Remove false matches on isbn (1.52 KB, patch)
2020-11-05 09:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 26364: Remove useless condition (974 bytes, patch)
2020-11-05 09:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 26364: Don't discard the passed isbn (1.05 KB, patch)
2020-11-05 09:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 26364: Adjust unit tests (2.69 KB, patch)
2020-11-05 09:50 UTC, David Nind
Details | Diff | Splinter Review
Bug 26364: Don't skip records that match the isbn we passed (921 bytes, patch)
2021-01-04 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26364: Remove false matches on isbn (1.58 KB, patch)
2021-01-04 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26364: Remove useless condition (1.01 KB, patch)
2021-01-04 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26364: Don't discard the passed isbn (1.11 KB, patch)
2021-01-04 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26364: Adjust unit tests (2.75 KB, patch)
2021-01-04 22:18 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-09-03 09:53:37 UTC
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
Comment 1 Nick Clemens 2020-09-03 09:59:27 UTC
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
Comment 2 Jonathan Druart 2020-09-03 14:13:30 UTC
Can you explain? Is it an alternative patch for bug 26270?
Comment 3 Jonathan Druart 2020-09-03 14:13:47 UTC
Where did you see it fail?
Comment 4 Nick Clemens 2020-09-03 14:56:48 UTC
(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
Comment 5 Nick Clemens 2020-09-03 15:45:47 UTC
(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
Comment 6 Nick Clemens 2020-09-03 15:47:33 UTC
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
Comment 7 Jonathan Druart 2020-09-07 08:51:43 UTC
> 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?
Comment 8 Nick Clemens 2020-09-09 14:36:54 UTC
Created attachment 109821 [details] [review]
Bug 26364: Don't skip records that match the isbn we passed
Comment 9 Nick Clemens 2020-09-09 14:36:57 UTC
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
Comment 10 Nick Clemens 2020-09-09 14:37:00 UTC
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?
Comment 11 Nick Clemens 2020-09-09 14:37:03 UTC
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
Comment 12 Nick Clemens 2020-09-09 14:37:06 UTC
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
Comment 13 Nick Clemens 2020-09-09 14:39:39 UTC
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
Comment 14 David Nind 2020-11-05 09:50:04 UTC
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>
Comment 15 David Nind 2020-11-05 09:50:07 UTC
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>
Comment 16 David Nind 2020-11-05 09:50:11 UTC
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>
Comment 17 David Nind 2020-11-05 09:50:16 UTC
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>
Comment 18 David Nind 2020-11-05 09:50:21 UTC
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>
Comment 19 David Nind 2020-11-05 09:52:10 UTC
To test, I followed the test plans in comment #13, #15 and #18.
Comment 20 Katrin Fischer 2021-01-04 22:18:00 UTC
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>
Comment 21 Katrin Fischer 2021-01-04 22:18:04 UTC
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>
Comment 22 Katrin Fischer 2021-01-04 22:18:08 UTC
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>
Comment 23 Katrin Fischer 2021-01-04 22:18:11 UTC
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>
Comment 24 Katrin Fischer 2021-01-04 22:18:15 UTC
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>
Comment 25 Katrin Fischer 2021-01-04 22:18:59 UTC
(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.
Comment 26 Nick Clemens 2021-01-06 13:16:55 UTC
(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
Comment 27 Jonathan Druart 2021-01-07 14:37:42 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 28 Fridolin Somers 2021-01-08 06:46:40 UTC
Pushed to 20.11.x for 20.11.02
Comment 29 Andrew Fuerste-Henry 2021-01-14 13:14:37 UTC
Pushed to 20.05.x for 20.05.08
Comment 30 Victor Grousset/tuxayo 2021-01-22 00:58:07 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.