DEV-940/DEV-955 - simplify canister usage #40
Conversation
- remove preflight check for redirects file - add helper for overriding services in tests
| ENV["NO_DB"] == "1" or Services[:no_external_data?] | ||
| end | ||
|
|
||
| Services.register(:no_external_data?) do |
There was a problem hiding this comment.
Not used except in Services, so removed.
| HathiTrust::Services.register(:no_external_data?) { @save_no_external_data } | ||
| HathiTrust::Services.register(:redirects) { @save_redirects } | ||
| context "using nonexistent redirect file" do | ||
| override_service(:redirect_file) { "no_such_redirects_file.gz.txt" } |
There was a problem hiding this comment.
redirects itself should get re-composed after the dependent redirect_file gets updated. So long as redirect_file itself doesn't throw an exception, I don't think this should cause any stack issues.
spec/cictl/index_command_spec.rb
Outdated
| it "bails out" do | ||
| expect { | ||
| CICTL::Commands.start(["index", "all", "--no-wait", "--quiet", "--log", test_log]) | ||
| }.to raise_error(Errno::ENOENT) |
There was a problem hiding this comment.
IMO the "file not found" error this raises is clear enough.
| @@ -120,13 +112,17 @@ | |||
| end | |||
|
|
|||
| describe "#index today" do | |||
| # Create new update and delete files in a temp directory based on fixtures. | |||
| after(:each) { HathiTrust::Services.register(:data_directory) { @save_dd } } | |||
There was a problem hiding this comment.
The override_service helper doesn't work here, because we need to do part of the test with the original data_directory and then later override it. That might point to a need to separately define fixtures or something for the tests rather than doing everything with data_directory, but I don't think it's worth trying to untangle right now.
aelkiss
changed the title
There was a problem hiding this comment.
I'm happy with everything here, with one exception: removing the preflight check. That was added not because of the Canister stack issues, but because last time I was in the production code I noticed that cictl index all -- so far as I could tell -- would first purge the Solr index (solr_client.empty_records!) before getting around to checking the redirects file and only then bail out with an error, leaving an empty index. Even with us running this manually this is pretty sketchy behavior.
I'd like to see some kind of redirects check done early enough to prevent that.
|
Got it. I will see if I can add a test for that. |
|
Also regarding the redirects it doesn't look like we test the |
If the redirect file does not exist, can't be read, etc, then indexing can fail partway through.
|
I pushed another commit that adds back |
There was a problem hiding this comment.
I'm happy with this.
There's one irritation I have about saving and restoring Canister registrants (both in code I wrote and in override_service) and that is the inability to get the procs that were registered, only the results. To truly restore the Canister state, the original proc should be re-run post-restore and the result memoized again. But that would require an additional accessor built into Canister, and I dunno if it's worth it.
Edit: and I should note that this kind of thing only should pertain to running tests -- I don't expect to be saving/restoring Canister stuff in a production environment.
Agreed. Probably something to consider upstream for canister. I created mlibrary/canister#8, and also put that on my "someday/maybe" list - if it becomes a real problem in the future we can address if nobody else gets to it. |
Tests pass for me with no stack issues. I did occasionally see the stack issues in the process of cleaning this up, but not with respect to the redirects file not being found. I suspect it might have been happening when something in
Serviceswas overridden, but the test failed and the original value wasn't restored. Using the providedoverride_servicetest helper should avoid that (i.e. the original value will be restored even if the test raises an exception)@moseshll Assuming you're good with this, I'm good with the other branch then being merged to main.