Config: show vehicle asleep state instead of value errors#31859
Config: show vehicle asleep state instead of value errors#31859naltatis wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/vehicle-asleep.spec.ts" line_range="15-23" />
<code_context>
+});
+
+test.describe("asleep vehicle", async () => {
+ test("config: values show sleeping state instead of errors", async ({ page }) => {
+ await page.goto("/#/config");
+
+ const vehicle = page.getByTestId("vehicle");
+ await expect(vehicle).toHaveCount(1);
+ await expect(vehicle).toContainText("Sleepy Car");
+ await expect(vehicle.getByTestId("device-tag-capacity")).toContainText("68.0 kWh");
+ await expect(vehicle.getByTestId("device-tag-soc")).toContainText("sleeping");
+ await expect(page.getByTestId("header")).toBeVisible();
+ });
+});
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting on the absence of an error style and making the `"sleeping"` text assertion robust to localization.
The test correctly verifies that SoC shows "sleeping" instead of an error. To make it more robust against regressions, also assert that the SoC element lacks error-specific styling (e.g., `text-danger` or an error icon). Since the UI is translated, consider avoiding a hardcoded "sleeping" string by fixing the locale in the test environment or asserting using a more stable marker (such as a data attribute or test id plus absence of error text).
Suggested implementation:
```typescript
test.describe("asleep vehicle", async () => {
```
```typescript
test("config: values show sleeping state instead of errors", async ({ page }) => {
await page.goto("/#/config");
const vehicle = page.getByTestId("vehicle");
const socTag = vehicle.getByTestId("device-tag-soc");
await expect(vehicle).toHaveCount(1);
await expect(vehicle).toContainText("Sleepy Car");
await expect(vehicle.getByTestId("device-tag-capacity")).toContainText("68.0 kWh");
// SoC should indicate a sleeping state, not an error
await expect(socTag).not.toHaveClass(/text-danger/);
await expect(socTag.getByTestId("error-icon")).toHaveCount(0);
await expect(page.getByTestId("header")).toBeVisible();
```
To make the "sleeping" state assertion fully robust against localization, consider:
1. Adding a stable marker in the UI for the sleeping state (e.g. `data-state="sleeping"` or a dedicated `data-testid="device-tag-soc-sleeping"` on the SoC element) and updating the test to assert on that instead of any localized text.
2. If your app supports a test locale (e.g. forcing `locale=en` via configuration or URL), you can alternatively fix the locale in the test setup and keep a single text-based assertion.
3. Adjust the selectors used in the absence-of-error-style checks (`text-danger` class and `error-icon` test id) to match the actual error styling and DOM structure used in your application.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| var _ StringGetter = (*errorPlugin)(nil) | ||
|
|
||
| func (o *errorPlugin) StringGetter() (func() (string, error), error) { |
There was a problem hiding this comment.
See tests/vehicle-asleep.evcc.yaml. Returning an error from a custom getter.
There was a problem hiding this comment.
Why does soc need a stringgetter?
There was a problem hiding this comment.
This is the defaultGetter pattern also used in other plugins. Now added to error plugin.
https://github.com/evcc-io/evcc/blob/master/plugin/script.go#L51
https://github.com/evcc-io/evcc/blob/master/plugin/http.go#L78
There was a problem hiding this comment.
Yes- but why does this specific PR need it? It shouldn't or something strange is going on.
There was a problem hiding this comment.
To create a simple e2e test case. see tests/vehicle-asleep.evcc.yaml
| } | ||
| tr.Error = err.Error() | ||
| // asleep is a valid vehicle state, not an error | ||
| if errors.Is(err, api.ErrAsleep) { |
There was a problem hiding this comment.
If you want to make this additional just
tr.Asleep = errors.Is(err, api.ErrAsleep)
There was a problem hiding this comment.
Otherwise we cant distinguish (apart from error string checking) the sleep state from a "real" error.
| Error string `json:"error"` | ||
| Value any `json:"value"` | ||
| Error string `json:"error"` | ||
| Asleep bool `json:"asleep,omitempty"` |
There was a problem hiding this comment.
Why do we need this? ErrAsleep is a "normal" error that will be returned like other errors. If the UI doesn't want to display that it can match on the error property already.
There was a problem hiding this comment.
ErrAsleep is a "normal" error that will be returned like other errors.
No, in other parts of the codebase we explicitly check for the Asleep state and handle it. It's usually not logged to the UI like other errors.
fixes #31852
Vehicles reporting
api.ErrAsleepshowed red error values in the config device tile, although asleep is a regular vehicle state.asleep: trueinstead of an error