#Removing more fields from snapshot

1 messages ยท Page 1 of 1 (latest)

lean kayak
#

So I think the current snapshot is snapshotting too much stuff that isn't really relevant for the test

#
# name: test_all_entities[aeotec_home_energy_meter_gen5][sensor.aeotec_energy_monitor_energy-entry]
  EntityRegistryEntrySnapshot({
    'aliases': list([
      None,
    ]),
    'area_id': None,
    'capabilities': dict({
      'state_class': <SensorStateClass.TOTAL_INCREASING: 'total_increasing'>,
    }),
    'config_entry_id': <ANY>,
    'config_subentry_id': <ANY>,
    'device_class': None,
    'device_id': <ANY>,
    'disabled_by': None,
    'domain': 'sensor',
    'entity_category': None,
    'entity_id': 'sensor.aeotec_energy_monitor_energy',
    'has_entity_name': True,
    'hidden_by': None,
    'icon': None,
    'id': <ANY>,
    'labels': set({
    }),
    'name': None,
    'object_id_base': 'Energy',
    'options': dict({
      'sensor': dict({
        'suggested_display_precision': 2,
      }),
    }),
    'original_device_class': <SensorDeviceClass.ENERGY: 'energy'>,
    'original_icon': None,
    'original_name': 'Energy',
    'platform': 'smartthings',
    'previous_unique_id': None,
    'suggested_object_id': None,
    'supported_features': 0,
    'translation_key': None,
    'unique_id': 'f0af21a2-d5a1-437c-b10a-b34a87394b71_main_energyMeter_energy_energy',
    'unit_of_measurement': 'kWh',
  })
# ---
# name: test_all_entities[aeotec_home_energy_meter_gen5][sensor.aeotec_energy_monitor_energy-state]
  StateSnapshot({
    'attributes': ReadOnlyDict({
      'device_class': 'energy',
      'friendly_name': 'Aeotec Energy Monitor Energy',
      'state_class': <SensorStateClass.TOTAL_INCREASING: 'total_increasing'>,
      'unit_of_measurement': 'kWh',
    }),
    'context': <ANY>,
    'entity_id': 'sensor.aeotec_energy_monitor_energy',
    'last_changed': <ANY>,
    'last_reported': <ANY>,
    'last_updated': <ANY>,
    'state': '19978.536',
  })
# ---
#

Stuff like aliases, area_id, config_entry_id, config_subentry_id, device_id, id, labels, previous_unique_id, context, last_changed, last_reported and last_updated never really carry any value

lofty sparrow
#

I don't agree

#

On the contrary, I think we should snapshot more/smarter, to tie entites to devices, config entries and config subentries.

lean kayak
#

Agreed on that one

#

I am also open to other suggestions, but just having 1000 lines of 'id': <ANY>

lofty sparrow
#

so?

#

it's better than excluding it

lean kayak
#

I mean it does make me wonder why

lofty sparrow
#

is this triggered by Arturs PR which had to update all snapshots, or what's the context?

lean kayak
#

now we have a thing where we break CI more often because something that isn't related to what we're generally tested changed

#

ye

#

and it's not a direct high prio discussion

#

but it does make one wonder

lofty sparrow
#

ok, then I strongly disagree. for me the point of the snapshots is to capture accidental changes, and not have to opt-in to check everything

#

we do stuff like arturs PR now maybe 2-3 times per year, and yes, it's a minor inconvenience that snapshots in pending PRs are stale

#

but it's not worse than bumping a linter which changes the formatting

lean kayak
#

Yes, but I am wondering if we have snapshots where aliases wasn't an empty list

lofty sparrow
#

does it matter? it's included in the entity registry container

lean kayak
#

There are other things we omit already

lofty sparrow
#

meh

#

but take a look at trying to make sure the expected config entry etc is linked to

#

that would be actually useful

lean kayak
#
    @classmethod
    def _serializable_entity_registry_entry(
        cls, data: er.RegistryEntry
    ) -> SerializableData:
        """Prepare a Home Assistant entity registry entry for serialization."""
        serialized = EntityRegistryEntrySnapshot(
            attrs.asdict(data)
            | {
                "config_entry_id": ANY,
                "config_subentry_id": ANY,
                "device_id": ANY,
                "id": ANY,
                "options": {k: dict(v) for k, v in data.options.items()},
            }
        )
        serialized.pop("categories")
        serialized.pop("compat_aliases")
        serialized.pop("compat_name")
        serialized.pop("_cache")
        serialized["aliases"] = er._serialize_aliases(serialized["aliases"])
        return cls._remove_created_and_modified_at(serialized)
lofty sparrow
#

should we also not bump linters because it's annoying for CI?

#

anyway, I've made my point clear. I'll not debate anymore.

lean kayak
#

Oh I am not saying that we shouldn't do it, it's a minor inconvenience at best

#

but if all the aliases are [None], what's the point in storing it and creating the inconvenience in the first place

lofty sparrow
#

because if someone wants to test aliases, it will just work

#

I think it's weird categories is not included btw

lean kayak
#

I am wondering if it makes sense to have a separate snapshot serializer?

lofty sparrow
#

why not? any conversation integration would want to test that

#

anyway, I said I would stop debating ๐Ÿ˜ฌ

lean kayak
#

At this point I want to try to understand your stance and when you think this would be useful ๐Ÿ™‚

lofty sparrow
#

I already replied to that:

if someone wants to test aliases, it will just work

#

For example in a conversation or voice assistant related test