Testing feature properties against field definitions #152
Conversation
…, fixed data and sources
|
@nickpeihl I did this change against |
💚 Build Succeeded |
|
@nickpeihl I can also address #53 easily changing the test to make properties keys and field names fully equal but then we have three cases where some labels are not present as they were marked as optional in the SPARQL queries. The countries are
We can remove those labels, or maybe add a fallback value in the queries, just ignore those fields in the test, or skip this requirement. My initial thought is that since they were marked as Maybe we can add a new property for the field definition that marks it as optional, so the test can take it into account, but also any EMS client in the future? |
There was a problem hiding this comment.
Thanks @jsanz. I can not decide if we should do this or not. It does make the vector files more consistent, but I do not know if it is worth the changes we have to make to the existing data.
I would appreciate any additional thoughts you have.
…ps and field definitions are aligned
💚 Build Succeeded |
Thanks @jsanz. I can not decide if we should do this or not. It does make the vector files more consistent, but I do not know if it is worth the changes we have to make to the existing data. I don't like much adding more complexity to the schema, since there are no external requirements for this. Still, I like having the chance to check this, so my last commit added the check only when an environment variable is present. That way the default behavior/contract is to check one way but still the way back can be also tested without breaking current CI, at least for the moment. Since it's a very trivial change I can revert this one if we agree in a different approach independently of the changes in the data around that commit 😅. I will also remove the property from South Korea and the US changes you suggested. Thanks for taking a look!! |
💚 Build Succeeded |
In elastic#152 we changed the `name` field to `label_en` in the USA States layer in EMS 8+ to match the precedent with other country subdivisions. To make this change transparent to users, we need to have a saved object migration for term joins, styles, and tooltips that use the `name` field. This PR proposes we delay the changes to the USA States layer to give us more time to implement the saved object migration in Kibana. With a proper migration it should be possible to implement this layer change in a minor EMS release (e.g. 8.1). In that case we could change the `versions` in the `states_v1.hjson` and `states_v8.hjson` configs to be `1 - 8.0` and `>= 8.1` respectively.
In #152 we changed the `name` field to `label_en` in the USA States layer in EMS 8+ to match the precedent with other country subdivisions. To make this change transparent to users, we need to have a saved object migration for term joins, styles, and tooltips that use the `name` field. This PR proposes we delay the changes to the USA States layer to give us more time to implement the saved object migration in Kibana. With a proper migration it should be possible to implement this layer change in a minor EMS release (e.g. 8.1). In that case we could change the `versions` in the `states_v1.hjson` and `states_v8.hjson` configs to be `1 - 8.0` and `>= 8.1` respectively.
fixes #151
Improving the test to check for every feature that all its properties are included in the source field definition. I also included a script I had on hold since it proved useful to fix the failing tests raised by this improvement.
With this new check I fixed also:
nutsprop inside