Expand and modularize concerto test data set

Bug #1066888 reported by Bill Erickson
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Wishlist
Unassigned

Bug Description

As a part of the push to improve Evergreen testing, this bug is meant to track additions to the concerto test SQL data set. My goals for now are to add data, make it simpler to plug in new test data, and to allow users to load different sets of test data, depending on the their needs (e.g. load only the concerto set as opposed to all of the bibs and users, etc.)

Much of this is already done, pending peer review. However, I'd still like to add some more data.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/berick/concerto-expand-modularize

Bill Erickson (berick)
description: updated
Revision history for this message
Bill Erickson (berick) wrote :

I've pushed a number of small tweaks. Today I also pushed code for loading test circulations. Current data set loads 162 circs. More would be good....

Revision history for this message
Dan Scott (denials) wrote :

Hi Bill:

Finally reviewing this - I like where you're taking things! A couple of thoughts with an eye towards using these in tests sooner rather than later:

1) Rather than using /* ... */ comments for the functions, why not use COMMENT ON FUNCTION - that way we can see the comments down the road right in the database.

2) It would be nice to mix in some non-ASCII user data into the names and addresses to ensure searching & displaying work as expected.

3) At least some of the users should have NULL dates of birth (many sites don't keep DOBs for their patrons) and rather than empty strings for suffixes we should probably use NULLs in most cases. (Having a few empty strings will add to the realism though).

4) Some (or perhaps most?) of the usrnames should be non-numeric, so that we can exercise code that determines barcodes vs. usernames. Perhaps we can reuse the passwords for this purpose?

5) This is my fault to begin with, but we should really use Dewey and LC call numbers so that we can properly test sorting and call number browsing.

I'll be happy to help out with these, following the rule of he who raises concerns gets to work on them... To reiterate, I really like what you're doing with this. Having the circ transactions in the mix really raises the bar!

Revision history for this message
Bill Erickson (berick) wrote :

Thanks for the feedback, Dan. Agreed on all points.

For #4, the staff accounts all have non-numeric usernames. Doing the same for some/most of the patrons would be good, too, though.

I'll get started on #1, then on to #3 and see where that takes me.

Revision history for this message
Bill Erickson (berick) wrote :

For #1, the /* comments are all linked to temporary evergreen.* tables and functions, which are deleted after the test data import. We could keep them around, but I'm not sure if we want/need to.

Revision history for this message
Bill Erickson (berick) wrote :

Pushed change to set most DOBs to NULL and most suffixes to NULL. A few suffixes remain as '' and others are (still) more realistic.

Revision history for this message
Dan Scott (denials) wrote :

Good point on the temporary functions getting dropped; I hadn't considered that. I guess from the perspective of encouraging people to look at what we're doing there's a chance of edjumacating them or influencing them if they copy/paste from the examples, but it's far from a must-have to switch over.

Revision history for this message
Bill Erickson (berick) wrote :

Pushed:

---
For the first 20 users in each patron group and the first 3 users in each staff group:

     * create 3 regular and 3 overdue circs
     * create 3 title holds, one of which is frozen
     * if the user is in a Staff group, also create one copy-level hold.
---

Revision history for this message
Thomas Berezansky (tsbere) wrote :

Looks like all of the requestors for the holds are the admin user (from a quick glance, anyway). Perhaps some of the holds should be "user placed"? Just a thought.

Revision history for this message
Bill Erickson (berick) wrote :

Indeed, Thomas. There are quite a few default values being used throughout. It's helpful to know what is most important to address. Thanks.

Revision history for this message
Bill Erickson (berick) wrote :

Pushed:

    * Make most hold requests self-placed instead of admin-placed
    * 1 hold for each user is placed by a Circulator
    * All circulations now use a Circulator user as the circ_staff

Circulator for each is first by ID with same home_ou as user

Revision history for this message
Dan Scott (denials) wrote :

Pushed a commit onto the collab branch that uses real LC and Dewey call numbers as a base for the concerto bibs, and adds the ability to set the class when using the evergreen.populate_callnumber() function.

One consquence of breaking this out into multiple files is that I used to just run "psql -f Open-ILS/tests/databases/concerto.sql" but now I have to cd into the directory first. A small inconvenience, to be sure.

Revision history for this message
Dan Scott (denials) wrote :

Added two more commits:

1) Enable the French bibs to get assets, as populate_callnumber() didn't understand that a quoted value should be interpreted as a TEXT parm. Okay. Could have casted the parms, I guess, but I just added a NULL parm to invoke the 4-arg version instead. (Note: there's a chance I introduced the problem myself by adding the explicitly 3-parm variant of evergreen.populate_callnumber(); more testing would bear this out).

2) More interesting: address the inconvenience I was whining about in the previous comment by teaching eg_db_config.pl to load the sample datasets, using the --load-all-sample / --load-concerto-sample arguments. I *think* deploying to a remote PostgreSQL server will fail due to the "\i " psql commands, but I could be wrong - don't have a setup to test with at the moment. In any case, I've documented it as only working with a local PostgreSQL server. This should make setting up a reproducible testing scenario much simpler: invoke "eg_db_config.pl --create-database --create-schema --load-all" and Bob's your uncle.

Revision history for this message
Thomas Berezansky (tsbere) wrote :

After a very basic testing I am fairly certain that \i is applied by the client, not the server, and thus the files need not be remote. It is not like "read from this file" commands that the server applies. To my knowledge this is largely the case for all \ commands in psql, though some get converted into remote commands by psql itself.

Revision history for this message
Dan Scott (denials) wrote :

Based on Thomas' assertion I have added a commit to the collab branch that removes the documented restriction. Once we're ready to commit this stuff, I guess we can squash some commits while we're working out how to sign off things appropriately.

Revision history for this message
Bill Erickson (berick) wrote :

Pushed squashed (for my commits) and sign-offs (for dan's) branch to

working => collab/berick/concerto-expand-modularize-squash

tags: added: pullrequest
Changed in evergreen:
milestone: none → 2.4.0-alpha
assignee: Bill Erickson (erickson-esilibrary) → nobody
importance: Undecided → Wishlist
Revision history for this message
Dan Scott (denials) wrote :

Signed off on Bill's commits and squashed some of my own down, added in a release notes entry, and pushed to master. Thanks Bill!

Changed in evergreen:
status: New → Fix Committed
Ben Shum (bshum)
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.