-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test_get_corsika_telescope_list takes too long #1130
Comments
The The number of calls to the DB are reduced by having the Obviously always good to check that this is still working. |
The main difference to the old style is that now we call the DB for each telescope separately. In the past we read all of the parameters of all of the telescopes in one query (which is very quick). I suggest to extend the caching functionality to do that as well with the new style. Usually the model parameter version does not change from one telescope to the next (at least for now), so when the first telescope is read, we can fill the cache with the parameters of all telescopes for that same version. The should reduce the DB calls to one in most cases. |
I can confirm that when running at DESY this test takes 1 second, while running it at home takes 15-20 seconds. |
Started to have a look:
Given that we want to test the code and not the database connection, my suggestion is to change also this function and either use the Additionally:
|
I find the comments/suggestions a little bit contradictory. I will write comments below. They are out of order, but I think follow some logic still.
If we want to change it to be an integration test, then I wouldn't use mocking but actually connect to the DB as expected in integration tests and as we do in others.
This test actually accomplished that in a way. It showed that we are accessing the DB for each telescope separately, which is something we can probably easily avoid. I agree that having a dedicated test for accessing the DB is good though, let's try to come up with it after we decide on the structure of the DB.
It's fine to change the unit test to use mocking, but then I would add a separate integration test that does the same, actually using the DB. I think it's important to test this in a real scenario, considering the chain of "events" this test triggers (even though it is just one command). |
Oh and I would wait before implementing any changes until we implement the DB restructuring. This problem might resolve itself then because we will probably access the DB once, getting the entries based on the table of production version/parameter versions. |
Yes and no:
I think both points are not affected by restructuring. |
Yes, sorry, I didn't mean that those points would not be valid anymore, just that this specific issue of many unnecessary calls would (hopefully) be resolved. I don't think we should add another cache at the moment (like I proposed above) because it will probably be superfluous once we implement the restructuring. |
The
test_get_corsika_telescope_list
test takes roughly 20 seconds to run. Profiling it points to excessive time spent initializing the array model, perhaps related to reading from the DB too many times.Need to look into it further (in the middle of something else, opening this issue so it is not forgotten).
The text was updated successfully, but these errors were encountered: