Skip to content
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

Handle LastFM unavailable #94456

Merged
merged 21 commits into from
Jun 23, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 22 additions & 20 deletions homeassistant/components/lastfm/sensor.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

import hashlib

from pylast import LastFMNetwork, Track, User, WSError
from pylast import LastFMNetwork, PyLastError, Track, User
import voluptuous as vol

from homeassistant.components.sensor import PLATFORM_SCHEMA, SensorEntity
Expand Down Expand Up @@ -105,25 +105,27 @@ def __init__(self, user: User, entry_id: str) -> None:
def update(self) -> None:
"""Update device state."""
try:
self._user.get_playcount()
except WSError as exc:
play_count = self._user.get_playcount()
except PyLastError as exc:
self._attr_available = False
LOGGER.error("Failed to load LastFM user `%s`: %r", self._user.name, exc)
return
self._attr_entity_picture = self._user.get_image()
if now_playing := self._user.get_now_playing():
self._attr_native_value = format_track(now_playing)
else:
self._attr_native_value = STATE_NOT_SCROBBLING
top_played = None
if top_tracks := self._user.get_top_tracks(limit=1):
top_played = format_track(top_tracks[0].item)
last_played = None
if last_tracks := self._user.get_recent_tracks(limit=1):
last_played = format_track(last_tracks[0].track)
play_count = self._user.get_playcount()
self._attr_extra_state_attributes = {
ATTR_LAST_PLAYED: last_played,
ATTR_PLAY_COUNT: play_count,
ATTR_TOP_PLAYED: top_played,
}
try:
self._attr_entity_picture = self._user.get_image()
if now_playing := self._user.get_now_playing():
self._attr_native_value = format_track(now_playing)
else:
self._attr_native_value = STATE_NOT_SCROBBLING
top_played = None
if top_tracks := self._user.get_top_tracks(limit=1):
top_played = format_track(top_tracks[0].item)
last_played = None
if last_tracks := self._user.get_recent_tracks(limit=1):
last_played = format_track(last_tracks[0].track)
self._attr_extra_state_attributes = {
gjohansson-ST marked this conversation as resolved.
Show resolved Hide resolved
ATTR_LAST_PLAYED: last_played,
ATTR_PLAY_COUNT: play_count,
ATTR_TOP_PLAYED: top_played,
}
except PyLastError:
return
Copy link
Member

@frenck frenck Jun 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a large try scope? As in, if get_top_tracks fails, a user is left with a partial update? While get_recent_tracks isn't even tried anymore?

The scope of the try should be limited to a single call IMHO; or handle the unavailable state (in which all values become unavailable).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I look at my logs this happened like 20 times in the last 4 days. What should the preferred option be? In my opinion, the best way would be to split the data up into several sensors so we can put it unavailable independently, but that'd be a breaking change. So would putting everything in one try except be the best solution for now?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would implement the available state right now, and make the sensor unavailable. As a follow-up PRs:

  • Implement a data coordinator
  • Add sensor descriptions
  • Add separate sensors for the attributes
  • Keep the attributes for the entity around for a couple of release cycles before removing them from the old sensor entity.

21 changes: 21 additions & 0 deletions tests/components/lastfm/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,27 @@ def get_friends(self) -> list[any]:
return self._friends


class FailingMockUser(MockUser):
"""Mock user built to pass verification but raise exception later."""

amount_called = 0

def __init__(self):
"""Initialize default user."""
super().__init__(
now_playing_result=Track("artist", "title", MockNetwork("lastfm")),
top_tracks=[Track("artist", "title", MockNetwork("lastfm"))],
recent_tracks=[Track("artist", "title", MockNetwork("lastfm"))],
)

def get_image(self) -> str:
"""Raise exception when get user image."""
self.amount_called += 1
if self.amount_called > 1:
raise PyLastError("network", "status", "Page not found")
return ""


def patch_user(user: MockUser) -> MockUser:
"""Patch interface."""
return patch("pylast.User", return_value=user)
Expand Down
36 changes: 34 additions & 2 deletions tests/components/lastfm/test_sensor.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
"""Tests for the lastfm sensor."""
from datetime import timedelta
from unittest.mock import patch

from pylast import WSError
Expand All @@ -16,11 +17,12 @@
from homeassistant.core import HomeAssistant
from homeassistant.helpers import issue_registry as ir
from homeassistant.setup import async_setup_component
from homeassistant.util import dt as dt_util

from . import API_KEY, USERNAME_1, MockUser
from . import API_KEY, USERNAME_1, FailingMockUser, MockUser
from .conftest import ComponentSetup

from tests.common import MockConfigEntry
from tests.common import MockConfigEntry, async_fire_time_changed

LEGACY_CONFIG = {
Platform.SENSOR: [
Expand Down Expand Up @@ -111,3 +113,33 @@ async def test_update_playing(
assert state.attributes[ATTR_LAST_PLAYED] == "artist - title"
assert state.attributes[ATTR_TOP_PLAYED] == "artist - title"
assert state.attributes[ATTR_PLAY_COUNT] == 1


async def test_update_failed(
hass: HomeAssistant,
setup_integration: ComponentSetup,
config_entry: MockConfigEntry,
default_user: MockUser,
) -> None:
"""Test handling exception after API is tested."""
await setup_integration(config_entry, FailingMockUser())

entity_id = "sensor.testaccount1"

state = hass.states.get(entity_id)

assert state.state == "artist - title"
assert state.attributes[ATTR_LAST_PLAYED] == "artist - title"
assert state.attributes[ATTR_TOP_PLAYED] == "artist - title"
assert state.attributes[ATTR_PLAY_COUNT] == 1

future = dt_util.utcnow() + timedelta(minutes=15)
async_fire_time_changed(hass, future)
await hass.async_block_till_done()

state = hass.states.get(entity_id)

assert state.state == "artist - title"
assert state.attributes[ATTR_LAST_PLAYED] == "artist - title"
assert state.attributes[ATTR_TOP_PLAYED] == "artist - title"
assert state.attributes[ATTR_PLAY_COUNT] == 1