-
Notifications
You must be signed in to change notification settings - Fork 36
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
Add tests for calculated obsCollection #1373
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is child places needed by calculator?
childPlaces, err = FetchChildPlaces( | ||
ctx, store, metadata, httpClient, remoteMixer, ancestor, childType) | ||
if err != nil { | ||
return nil, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to always fetch this since the obsCollection collection could already exists from BT cache.
// If data is missing, attach empty child places. | ||
for _, place := range childPlaces { | ||
obsByEntity[place] = &pbv2.EntityObservation{} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
feels like this should be part of Flume logic. Right now Flume does not return place with no data. This is a behavior discrepancy.
I actually think it makes sense to not have an entry for a child place if there is no data in all scenarios.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add this to the flume instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think we should add places with no data. Imagine fetching cities in USA for a custom DC variable which has data for 10 places. That would have thousands of empty entries.
I think we should gvc and understand why the child places are needed..
The calculator uses a variable/entity to try and fetch the data for for the calculated response. Currently, for BT it will return no response for places with no data, so the calculator won't detect there's any missing variable/entity to compute |
update on this PR? @n-h-diaz |
This change also adds empty place responses for all the childPlaces, since the child places are needed by the calculator
This mirrors the response for other ObservationResponses where requested places without data are empty, but let me know if you'd like me to remove these