Verbesserte Nachberechnung der Energieverfolgung

@pierre-gilles ,

Ich greife das Thema auf, ich habe die Überprüfung für den PR zur Verbesserung der Nachberechnung der Energieverfolgung abgeschlossen. Ich denke, du kannst das überprüfen:

Danach folgt der nächste PR, der eine klarere Protokollierung für den Benutzer hinzufügt, insbesondere um zu sehen, wo die letzte Aufgabe stehen geblieben ist, falls ein Gladys-Neustart oder ein Problem während einer Nachberechnung aufgetreten ist (was verhindert, dass die gesamte Nachberechnung wiederholt werden muss …):

Könntest du genauer beschreiben, was dieser PR macht?

Da es den Kern betrifft, handelt es sich um recht sensible Änderungen.

Ich würde gerne genau den Umfang verstehen, um die Auswirkungen korrekt bewerten zu können.
Während der Entwicklung habe ich mehrere Monate damit verbracht, die Berechnungen zuverlässiger zu machen, und es ist für mich entscheidend, dass die Berechnung nicht beeinträchtigt wird :slight_smile:

Du hast Recht, die Details zu fragen, ich verstehe das vollkommen. Und wenn möglich, würde ich es auch gerne von dir testen lassen, indem du es neben deiner Produktion laufen lässt, um es gut zu überprüfen. Das ist das, was ich gemacht habe, um die ordnungsgemäße Funktion zu bewerten. Ich habe keine Fälle gefunden, in denen es abstürzt. Aber zum Beispiel habe ich keine Zigbee-Energie. Auch wenn es keinen Grund gibt, dass es anders sein sollte, würde es alles abdecken.

Der globale Diff ist groß (+3203 / -428), aber der funktionale Umfang, den ich in diesem PR anstrebe, ist vor allem dieser:

  1. Gezieltes Recalculating (Backend) für Datumsbereich / Feature-Auswahl
  • Hinzufügen des Recalculating für einen Datumsbereich für:
    • die Kosten (calculate-cost-range)
    • den Verbrauch ab Index (calculate-consumption-from-index-range)
  • Die Endpunkte „from beginning“ akzeptieren jetzt auch eine Auswahl von Features (feature_selectors).
  • Validierung der Daten YYYY-MM-DD auf der Controller-Seite (mit BadParameters, wenn das Format ungültig ist).
  1. Teilweises Recalculating ohne den Rest zu brechen
  • Wenn ich einen Zeitraum neu berechne, lösche ich nur die Zustände in diesem Zeitraum (destroyStatesBetween) vor der Neuberechnung.
  • Wenn es kein Enddatum gibt, behalte ich das Verhalten „from start → now“ (destroyStatesFrom).
  • Für den Verbrauch ab Index stelle ich ENERGY_INDEX_LAST_PROCESSED am Ende der Verarbeitung bei teilweisen/ausgewählten Neuberechnungen wieder her, um den globalen Cursor nicht zu beschädigen.
  1. Ausführung im Hintergrund
  • Umstellung auf wrapperDetached für lange Recalculating (from beginning / range), um sofort eine job_id zurückzugeben.
  • Das Frontend betrachtet die Aktion nur dann als OK, wenn eine job_id tatsächlich zurückgegeben wird.
  1. Frontend (Energy-Monitoring-Bildschirm)
  • Hinzufügen der Start- und Enddatumsfelder (jedes Feld ist optional).
  • Hinzufügen der Mehrfachauswahl von Features zum Recalculating.
  • Wenn keine Features ausgewählt sind, explizite Bestätigung vor der globalen Neuberechnung.
  • Automatischer Aufruf der range-Endpunkte, wenn ein Datum angegeben ist, andernfalls der from-beginning-Endpunkte.
  1. Was ich im Kern der Berechnung nicht verändert habe
  • Ich habe die Berechnungsformeln der Verträge nicht geändert (der Motor contracts.calculateCost bleibt derselbe).
  • Ich habe die Grundlogik des Index-Deltas nicht geändert (die Formel bleibt identisch), ich habe hauptsächlich den Umfang eingegrenzt (Selektoren, Daten, gezielte Löschung, Job-Handling).

Ich habe:

  • die Geschäftsformeln zur Berechnung der Verträge nicht geändert
  • das Prinzip des Index-Deltas nicht geändert
  • die Umrechnung der Einheit, die wir kürzlich eingeführt haben, nicht geändert.

Ich habe nur den Eingabebereich (ausgewählte Features, Datumsbereich) und die technische Einrahmung der Neuberechnungen (gezielte Löschung, Jobs, API/Frontend) modifiziert.

Und um es dir ganz zu sagen, Anfang der Woche hätte ich fast die PRs zu diesem Thema geschlossen und dir geschrieben, dass ich dir die Zügel überlasse.

Ich hatte Angst, dass es dir mehr Zeit in Anspruch nehmen würde, es zu überprüfen, als es selbst zu machen. Dass es dir optisch vielleicht nicht zusagt. Und dass du es deshalb wahrscheinlich besser selbst so machen würdest, wie du es dir vorstellst.

Aber ich schwankte zwischen dieser Frage und der Tatsache, dass wir trotzdem einige sind, die warten. Und der Tatsache, dass ich sehe, dass du mit anderen Dingen beschäftigt bist.

Also bin ich vom Post ausgegangen, dir die Wahl zu lassen. Und du gibst mir dein Feedback / deine Meinung. :wink:

Danke für die Präzisierung, ich halte dich auf dem Laufenden, wenn ich es mir angeschaut habe :wink:

Ich habe mir den PR schnell angesehen und habe den Eindruck, dass er mehr als nur die Neuberechnung mit den Daten betrifft.

Zum Beispiel scheinen die ersten vier Dateien ein Relikt eines anderen PR zu sein:

Was den Geschäftslogik-Code betrifft, ist der PR ziemlich umfangreich und betrifft so viele Elemente, dass es schwierig ist, sicher zu sein, dass er den Berechnungsalgorithmus nicht verändert.

Es gibt sogar eine neue Logik, die mit neuen Konzepten (wie shouldRestoreLastProcessed) eingeführt wird, also ist das eindeutig kein trivialer PR.

Trotzdem, intuitiv, wenn ich zwei Minuten darüber nachdenke, habe ich Schwierigkeiten zu verstehen, warum es so komplex ist.

Ich frage mich, ob deine Intuition nicht doch richtig ist:

Das ist wirklich schade, denn ich bin eher einverstanden mit dem Ergebnis und die Benutzererfahrung scheint gut zu sein. Das Problem ist, dass der Code riesig ist: Ich kann ihn nicht mergen, ohne ihn vollständig zu lesen, die neuen Konzepte zu verstehen und alle notwendigen Tests durchzuführen… :sweat_smile:

Was denkst du?

Hallo @pierre-gilles

Danke, dass du damit angefangen hast.
Der PR-Branch basiert auf dem Tasmota-Branch, der aufgrund unserer Diskussionen einige Änderungen erfahren hat.

  • Nach dem Update der PR, nachdem Tasmota gemerged wurde, habe ich eine weitere Überarbeitung vorgenommen, aber ich habe die Datei ‹ front/src/components/device/index.js › übersehen, die immer noch die Erstellung von Geräteverbrauch/Kosten enthält => Ich verstehe nicht, wie ich das übersehen konnte, es ist die erste Datei und ein großer Block. Es wurde entfernt

  • front/src/components/device/UpdateDevice.jsx und front/src/components/device/UpdateDeviceFeature.jsx waren die Hinzufügung eines Shortcuts von den Features, die Verbrauch/Kosten hatten, um direkt zur Energieverfolgungsseite zu gelangen. Das kann später vorgeschlagen werden, ich habe es entfernt.

  • front/src/components/drag-and-drop/DeviceListWithDragAndDrop.jsx: Die Komponente wird bei der Auswahl der Geräte zur Neuberechnung verwendet. Es war notwendig, die Möglichkeit zu entfernen, den Namen für diesen Zweck zu ändern.

Ich stimme zu, dass dies in diesem Punkt kein trivialer PR ist.

Grundsätzlich habe ich in Betracht gezogen, die Logik in einem separaten Rechenmotor zu duplizieren, um nicht von ENERGY_INDEX_LAST_PROCESSED abhängig zu sein.
Aber ich habe das absichtlich vermieden: Wir hätten zwei parallele Implementierungen (inkrementell vs. Neuberechnung) gehabt, also mehr Komplexität mit dem Risiko von Divergenz im Laufe der Zeit.

Heute habe ich calculateConsumptionFromIndexRange als Orchestrator (Datumsgrenzen, Reinigung, Fensterschleife) hinzugefügt.
Ich wollte die eigentliche Berechnung jedes Fensters beibehalten, indem ich immer calculateConsumptionFromIndex verwende, das ENERGY_INDEX_LAST_PROCESSED verwendet.

Daher dient shouldRestoreLastProcessed dazu, den inkrementellen Live-Cursor während der teilweisen Neuberechnungen/vergangenen Perioden zu schützen. Denn vorher ging die Berechnung immer « bis zum Ende ». Aber das Ziel ist es, einen alten Vertragspreis hinzuzufügen und die Neuberechnung nur auf diesem Bereich zu starten. Wenn das vor einem Jahr war, hätte der nächste Berechnungsschritt von 30 Minuten sonst eine komplette Jahresberechnung wieder aufgenommen, anstatt beim letzten echten Lauf der Verfolgung neu zu starten.

Konkrete Beispiel

  • Heute ist der 07/03 um 09:25 Uhr.
  • Manuelle Neuberechnung eines Geräts vom 01/03 bis 06/03.
  • Der automatische Job um 09:30 ist in der Warteschlange (Queue 1).

Ohne Wiederherstellung:

  • Der Cursor kann auf einem alten Wert bleiben (z. B. 06/03 23:30),
  • dann kann der Job um 09:30 zu weit bis 09:30 aggregieren.

Mit Wiederherstellung:

  • Wir setzen den Anfangswert des Cursors am Ende des Laufs zurück,
  • der Job um 09:30 startet auf einer konsistenten Basis.

Denn dieser Cursor wird für die Indexverbrauchsberechnung verwendet.

Kurz gesagt, es ist eine Sicherheitsmaßnahme, um einen einzigen wartbaren Motor zu behalten, ohne Nebenwirkungen auf die folgenden inkrementellen Läufe.

Natürlich könnte ich mich in der Logik irren, aber insbesondere an diesem Punkt habe ich mit der KI gearbeitet und versucht, das in alle Richtungen zu drehen, um sicherzustellen, dass es eine gute Vorgehensweise ist (nicht unbedingt die beste, die Review dient dazu!)

Ich bin mir dessen bewusst, und ich habe dir das von Anfang an in diesem PR gesagt. Es betrifft ein sensibles Element, und ich hoffte, dass du es auf einer Instanz neben deiner Produktion laufen lassen könntest, um sicherzustellen, dass die Berechnungen (die nicht verändert wurden) auch nach der Neuberechnung auf Bereichen usw. gut sind.
Was den Code betrifft, sind die größten Tests:

Gutes Wochenende und gute Lauf :wink:

Hallo @pierre-gilles,

Da ich nicht weiß, ob du gerade im Unterricht bist oder wann du diese PR überprüfen wirst, dachte ich, es wäre gut, eine vollständige Überprüfung von @coderabbitai anzufordern, da ich festgestellt habe, dass er nach einigen Durchgängen keine weiteren Überprüfungen mehr durchführt.

Und angesichts der Anzahl der Commits seit seiner ersten Überprüfung dachte ich, es wäre gut, eine vollständige Überprüfung der gesamten PR durchzuführen: https://github.com/GladysAssistant/Gladys/pull/2413#pullrequestreview-3943430667

Ich denke, er hat einige gute Dinge herausgearbeitet. Wenn du deine Überprüfung noch nicht begonnen hast, kann ich dann die Änderungen pushen? Oder was würdest du bevorzugen?

Ich habe es noch nicht angesehen, du kannst die PR gerne ändern :slight_smile:

Okay, das ist erledigt.

Hallo @pierre-gilles,

Ich melde mich nochmal wegen der PR #2413 (Energie-Neuberechnung).

Ich warte immer noch auf diese Neuberechnung, um die Energieverfolgungsintegration vollständig nutzen zu können. Ich habe die PR aufgeräumt (Master gemerged, CodeRabbit-Korrekturen, Retests auf meiner Prod über fast 2 Monate ohne Abweichungen in den Berechnungen zur offiziellen Version). Ich habe sie wieder geöffnet.

Zur Erinnerung an den Diskussionsthread: Du hattest mir Anfang April gesagt, dass du das Thema lieber selbst neu machst, weil dir die PR zu groß und zu sensibel erschien. Ich hatte zugestimmt und die PR pausiert.
Um konkret voranzukommen, schlage ich vor, die PR in mehrere kleine, unabhängige PRs aufzuteilen, die nacheinander gemerged werden können. Du könntest sie in kleinen Teilen durchgehen, jede Stufe auf deiner Prod testen und bei Bedarf leicht zurückgehen:

  • PR1: destroyStatesBetween + seine Tests (reine Utility, isoliert, ~100-200 Zeilen).
  • PR2: Neuberechnung durch Auswahl von Features ab Anfang (ohne Datumsbereich).
  • PR3: Neuberechnung durch Datumsbereich (basierend auf PR1 und PR2).
  • PR4: UI-Anpassungen.

Du könntest PR1 und PR2 mergen, ohne die sensibelsten Konzepte zu verankern (shouldRestoreLastProcessed erscheint erst in PR3).

Wenn du diese Aufteilung akzeptierst, mache ich mich diese Woche daran. Wenn du lieber selbst mit einem klaren Zeitplan weitermachen möchtest, sag es mir und ich schließe die PR wieder. Was mich stört, ist das Fehlen einer Entscheidung und dass ich die Integration nicht vollständig nutzen kann. Ich glaube, andere Benutzer waren ebenfalls betroffen.

Danke für dein Feedback.

Hallo,

Das Thema ist vor allem eine Frage des Vertrauens :slightly_smiling_face:

Was mir an dem aktuellen Stand des PRs Probleme bereitet, ist, dass es die Logik der Berechnung grundlegend ändert. Letztes Jahr hat mich dieser Teil mehrere Monate in Anspruch genommen, um ihn für die drei Vertragstypen (Basis, Voll-/Teillastzeiten und Tempo) zu stabilisieren, mit vielen Tests an echten Daten von Dutzenden von Nutzern.

Normalerweise dienen die Unit-Tests genau dazu, Regressionsfehler zu erkennen. Aber heute, mit der KI, können die Tests selbst massiv automatisch verändert werden, was hier der Fall ist. Daher kann ich die Tests nicht mehr als zuverlässige Garantie betrachten. Die KI könnte sehr gut Regressionsfehler eingeführt und die Tests angepasst haben, um sie zu validieren.

Um diesen PR so zu mergen, müsste ich also den gesamten geänderten Code lesen und verstehen und dann die gesamte Validierungsphase von letztem Jahr wiederholen. Und leider habe ich nicht einmal mehr die Testdatensätze, die mir einige Nutzer damals geschickt hatten. Es ist also ein Projekt, das fast dem ursprünglichen Entwicklungsaufwand entspricht, keine einfache Iteration.

Im Gegensatz dazu könnte ein „front only“-PR, der einfach die Möglichkeit hinzufügt, den letzten Monat / die letzten 3 Monate / 6 Monate / 12 Monate neu zu berechnen, wahrscheinlich in weniger als einer Stunde gemerged werden, ohne eine aufwendige Validierungsphase, da der Geschäftslogik-Code nicht geändert würde.

Die Aufteilung in kleine PRs kann helfen, die Änderungen lesbarer zu machen, aber sobald man hier die Geschäftslogik anfasst, wird es auf jeden Fall eine große, unvermeidbare Validierungsphase geben.

Was denkst du darüber?

Und entschuldige, wenn das hart klingen mag, es ist wirklich nicht gegen deine Arbeit. Es ist vor allem so, dass ich mit meinem aktuellen Zeitplan solche großen Projekte sehr schwer bewältigen kann :sweat_smile: Ich würde gerne mehr Zeit für solche gründlichen Überprüfungen haben, aber das Projekt ist noch nicht an diesem Punkt.

@pierre-gilles,

Zunächst einmal vielen Dank für diese Antwort, die den Vorteil hat, vollständig und ehrlich zu deinen Einschränkungen zu sein.

Um dir vollständig antworten zu können und nicht nur « Ich bin mir meines Codes sicher », habe ich mir die Zeit genommen, eine gründliche Überprüfung des PR (Pull Request) am Nachmittag durchzuführen. Ich habe die Anforderung mit einer klaren Anweisung gerahmt: Überprüfen, dass jede Änderung notwendig, solide, gut durchdacht, ohne Rückschritte und vor allem, dass die Tests nicht verfälscht werden — einschließlich des absichtlichen Brechens des Codes, um sicherzustellen, dass die Tests Rückschritte tatsächlich erkennen (echte Mutationstests). Genau das ist der legitime Zweifel, den du bezüglich KI und Tests aufwirfst, und ich wollte eine überprüfbare Antwort darauf geben, keine bloße Behauptung.

Ich präsentiere dir unten das Ergebnis dieser Überprüfung, das bei dir faktisch und reproduzierbar ist.

1. Zu deinem zentralen Einwand: « Der PR verändert die Logik der Berechnung grundlegend »

Ich behalte meine vorherige Antwort bei, diesmal jedoch mit einer zeilenweisen Überprüfung. Die beiden Dateien, in denen die Geschäftslogik lebt, sind:

  • server/services/energy-monitoring/lib/energy-monitoring.calculateConsumptionFromIndex.js
  • server/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.js

Ich habe jede geänderte Zeile gelesen. Was streng genommen unverändert bleibt:

  • convertEnergyUnit(...) — Einheitenumrechnungen
  • contracts[contract](...) — alle Kostenformeln (Basis, HP/HC, Tempo)
  • Berechnung des Index-Deltas und Verwaltung des Zähler-Rücksetzens
  • Filterung der Preise nach Datum + EDF Tempo + subtract(30, 'minutes')
  • saveMultipleHistoricalStates / saveHistoricalState

Die einzigen Ergänzungen in calculateConsumptionFromIndex.js sind in einem if (selectorSet.size > 0) eingekapselt (Filterung durch Whitelist): deaktiviert bei fehlender Whitelist, daher wird das Legacy-Verhalten für die Live-Inkrementberechnung, die alle 30 Minuten läuft, strikt beibehalten.

In calculateCostFrom.js sind die Ergänzungen:

  • Parsing der Start-/Enddaten (neuer Eintrag für den Bereichsmodus)
  • Filterung nach Selectoren (eingekapselt in if (selectorSet.size > 0))
  • destroyStatesBetween, wenn endAt angegeben ist, andernfalls destroyStatesFrom (Legacy-Verhalten)
  • Ein try/catch pro Kostenfunktion (Robustheit: eine Funktion, die abstürzt, blockiert die anderen nicht)

Die Kostenformel selbst — diejenige, die du monatelang auf Basis/HP/HC/Tempo stabilisiert hast — wurde nicht um ein einziges Zeichen verändert. Das ist in wenigen Minuten mit git diff master HEAD -- server/services/energy-monitoring/lib/energy-monitoring.calculateCostFrom.js und einem Strg+F auf contracts[contract], convertEnergyUnit, energyPricesForDate überprüfbar.

2. Zu deinem Einwand bezüglich der Tests: « KI kann Rückschritte einführen und die Tests anpassen, um sie zu validieren »

Das ist eine völlig legitime Sorge, und genau deshalb habe ich echte Mutationstests durchgeführt: Ich habe temporär drei kritische Punkte des Codes gebrochen und die gesamte Suite erneut gestartet. Das Ziel ist es, zu überprüfen, dass die Tests den Rückschritt erkennen, nicht, dass sie bestehen mit dem Rückschritt.

Hier sind die drei getesteten Mutationen und das Ergebnis:

Angewandte Mutation Tests, die brechen
Validierung der Daten aus dem Controller entfernt reject invalid start date format + reject invalid end date format (2 Tests)
Whitelist-Filter des Live-Motors calculateConsumptionFromIndex entfernt skip consumption features not in whitelist selectors (1 Test)
finally-Block zur Wiederherstellung des Cursors ENERGY_INDEX_LAST_PROCESSED entfernt restore last processed value on selector-based recalculation + restore last processed value on window error (2 Tests)

Gesamt: 5 Tests brechen genau bei den richtigen Behauptungen. Die Tests sind kein KI-Greenwashing, sie erkennen tatsächlich Rückschritte bei kritischen Schutzmaßnahmen. Reproduzierbar bei dir: Es reicht, diese drei Blöcke zu kommentieren und npm run test-service --service=energy-monitoring erneut zu starten. Die Dateien wurden nach der Überprüfung wiederhergestellt, ich habe mit git status bestätigt, dass keine restlichen Änderungen vorhanden sind.

Ich habe auch die Nicht-Rückschrittigkeit der bestehenden Tests überprüft:

  • Die geänderten Legacy-Tests (in calculateConsumptionFromIndex.test.js) haben ausschließlich eine Ergänzung von undefined als zweitem Parameter (um sich an die neue Signatur anzupassen). Keine Behauptung wurde geschwächt oder entfernt.
  • calculateCostFromYesterday.test.js ist tatsächlich gestärkt: Früher überprüfte er nur calledOnce, jetzt überprüft er die exakte Signatur der übergebenen Argumente.
  • Controller-Tests: Das Muster try/catch + expect.fail wird durch next(error) ersetzt — das ist das korrekte Express-Muster.

3. Zu deinem Vorschlag « PR nur für Frontend mit Presets 1/3/6/12 Monate »

Ich verstehe die Logik: Vermeide die schwere Validierungsphase im Geschäftsbereich, indem du den Backend nicht anfasst. Aber dieser Vorschlag deckt den tatsächlichen Bedarf der Benutzer nicht ab, und es ist wichtig, dass ich das erkläre:

Anwendungsfall Nr. 1 — Historische Neuberechnung für bestehende Geräte, die nachträglich überwacht werden

Das ist die Versprechen der Energienachverfolgung in Gladys. Viele Benutzer (auch ich) haben seit mehreren Jahren Tasmota- oder Zigbee2mqtt-Steckdosen in ihrer Gladys-Instanz, manchmal mit 4 Jahren gespeicherten Indexdaten. Wenn man die Energieüberwachungsintegration für diese Geräte hinzufügt, muss man die Verbrauch- und Kostenberechnung über den gesamten verfügbaren Verlauf neu berechnen können, nicht nur über die letzten 12 Monate.

Heute ist der einzige Weg die globale Neuberechnung von Anfang an, die:

  • für alle Geräte gleichzeitig neu berechnet (und nicht nur für das, das gerade hinzugefügt wurde)
  • bei größeren Installationen nicht abgeschlossen wird (ein Fall, den ich erlebe und den andere melden)
  • die HTTP-Anfrage auf der Frontend-Seite während der gesamten Berechnungsdauer blockiert (wahrscheinlich der stille Rückschritt, der dazu führt, dass man « es funktioniert nicht » für große Installationen sagt)

Ein Preset « Letzte 12 Monate » löst keines dieser drei Probleme. Eine Auswahl nach Funktion + Datumsbereich löst alle drei.

Anwendungsfall Nr. 2 — Der rückwirkende Tarif

Ein Benutzer, der feststellt, dass er vor 18 Monaten seinen Tarif geändert hat und nur diesen Zeitraum neu berechnen möchte, ist mit einem 12-Monats-Preset blockiert.

Anwendungsfall Nr. 3 — Die Granularität

Wenn man 30 überwachte Geräte hat und nur eine Datenkorrektur für ein einzelnes Gerät über eine Woche durchführen möchte, ist eine globale Neuberechnung über 1/3/6/12 Monate unverhältnismäßig und riskant für die anderen Geräte.

4. Zu den Robustheitsverbesserungen, die durch den PR gebracht werden

Ich möchte auch zwei Punkte erwähnen, in denen der PR den Code sicherer macht als master:

  • wrapperDetached (neue Job-Primitive) entkoppelt die HTTP-Anfrage von der langen Berechnung. Früher blockierte die Neuberechnung von Anfang an die HTTP-Anfrage bis zum Ende der Berechnung, was bei großen Installationen zu Timeouts führte. Das ist wahrscheinlich die stille Ursache für die « Es funktioniert nicht »-Meldungen, die regelmäßig gemeldet werden.
  • Der try/finally-Block um die Wiederherstellung des Cursors ENERGY_INDEX_LAST_PROCESSED schützt die Instanz vor einem korrupten Zustand, wenn ein Fenster während der Neuberechnung abstürzt. In master, wenn ein Fenster abstürzt, bleibt der Cursor zerstört und die Live-Inkrementberechnung startet bei der nächsten Iteration von vorne.

5. Ehrliche Punkte, die ich vor dem Push korrigieren werde

Um dir nicht zu erzählen, dass alles perfekt ist, hat die Überprüfung auch drei kleinere Cleanups (nicht blockierend, ohne Risiko eines Rückschritts) aufgedeckt:

  1. Ein Fragment der Abwärtskompatibilität in calculateCostFrom, das keine Bedeutung mehr hat (alle internen Aufrufer verwenden bereits die neue Signatur).
  2. Eine etwa 70%ige Duplikation zwischen FromBeginning und Range, die ich faktorisieren kann.
  3. 2-3 « zufällig bestandene » Tests in calculateCostFrom.test.js, die nicht wirklich das testen, was sie behaupten (zu härten oder zu entfernen — die echten Geschäftstests bleiben in den ursprünglichen, unmodifizierten Tests).

Ich verpflichte mich, diese drei Punkte vor dem Aufteilen zu korrigieren.

6. Mein konkreter Vorschlag

Angesichts dessen schlage ich dir immer noch das Aufteilen in Unter-PRs vor, jedoch mit der Berücksichtigung deiner Sorge um « unverzichtbare Validierungsphase »:

  • PR1 : destroyStatesBetween + wrapperDetached + deren Tests. ~150-200 Zeilen. Keine Geschäftslogik, nur Hilfsfunktionen. Kann schnell gemerged werden, geringes Risiko.
  • PR2 : Neuberechnung durch Feature-Auswahl von Anfang an (ohne Datumsbereich). Der Whitelist-Filter ist in if (selectorSet.size > 0) gekapselt, daher bleibt das Legacy-Verhalten streng erhalten.
  • PR3 : Neuberechnung durch Datumsbereich + shouldRestoreLastProcessed. Hier liegt die „echte“ geschäftliche Überprüfung.
  • PR4 : UI-Anpassungen.

Bei PR3 — diejenige, die dir die meisten Validierungen abverlangt — bin ich bereit:

  • Dir reproduzierbare Testdaten aus meiner Produktion (anonymisierte SQL-Ausgabe) bereitzustellen, die du bei dir nachspielen kannst
  • Dokumentation im Code zu shouldRestoreLastProcessed hinzuzufügen
  • Dir eine Video-Demonstration des Verhaltens vor/nachher zu geben, falls das hilft

Falls du diese Aufteilung validierst, beginne ich diese Woche mit PR1. Falls du lieber möchtest, dass ich in der Warteschleife bleibe, lass es mich mit einem Zeithorizont — auch wenn er weit ist — wissen, und ich werde die PR wieder gelassen schließen.

Danke für deine Zeit, und entschuldige, falls die Antwort dicht ist — ich habe lieber Fakten geliefert als Behauptungen.

@pierre-gilles, Ergänzung zu meiner vorherigen Nachricht.

Um das, was ich über die Prüfung geschrieben habe, konkret zu untermauern, habe ich die Aufräumarbeiten durchgeführt, die ich angekündigt hatte.

Aufräumarbeiten an calculateCostFrom.test.js

Bei der Prüfung der durch diesen PR hinzugefügten Tests in dieser Datei (die ursprünglichen Tests in master wurden nicht angetastet) habe ich 5 problematische Tests von 6 gefunden:

  • 2 Tests wurden einfach entfernt:
    • ein Duplikat mit einem bereits in master vorhandenen Test (should handle case where no energy price is found for the device at the given date)
    • ein Test « should return null when start date is invalid type », der aus dem falschen Grund bestanden hat (vorzeitige Beendigung, weil es kein Gerät in der Datenbank gab, nicht weil er wirklich den invalid type getestet hätte). Der Controller filtert diesen Fall bereits vorher aus, daher ist das Testen dieses Fallbacks auf einer Ebene, auf der es nie in der Produktion auftritt, Over-Testing.
  • 3 Tests wurden mit dem Muster Echte DB (device.create() + db.duckDbBatchInsertState() + energyPrice.create()) neu geschrieben, wie alle ursprünglichen Tests der Datei in master. Die vorherigen Versionen verwendeten stattdessen Stubs, die die Tests weniger zuverlässig und inkonsistent mit dem Rest der Datei machten.

Ein Wort zur Abdeckung

Während der Prüfung habe ich auch festgestellt, dass der Test should handle case where no energy price is found for the device at the given date, der in master vorhanden ist, die Verzweigung, die er angeblich testet, tatsächlich nicht abdeckt. Technischer Grund: Sein energy_parent_id auf dem Cost-Feature verweist auf das Index-Feature statt auf das Consumption-Feature, daher scheitert das Matching früher im Code und die Verzweigung if (energyPricesForDate.length === 0) wird nie erreicht. Diese Verzweigung wurde tatsächlich nur durch einen meiner gestubbten Tests abgedeckt, den ich entfernen wollte.

Ich habe daher einen sauberen Test mit echter DB hinzugefügt, um diese Abdeckung zu erhalten. Ich habe den master-Test, der den Fehler aufweist, nicht geändert — ich habe mir verboten, Legacy-Code anzufassen.

Zusammenfassung

  • 167 → 166 Tests bestanden (-1 netto: -2 Entfernungen, +1 Hinzufügung für die verwaiste Verzweigung)
  • Codeabdeckung bei 99.61 % gehalten (codecov-Ziel 98.80 %)
  • Diff vs master: -49 Zeilen in dieser Datei
  • Testmuster vereinheitlicht: 100 % echte DB bei den Hinzufügungen wie beim Legacy-Code
  • Kein ursprünglicher master-Test geändert

Ich habe eine Review für das PR 1 gemacht!

Meine Feedback: https://github.com/GladysAssistant/Gladys/pull/2528#pullrequestreview-4311385379

Danke dir,

Ich habe dir geantwortet und es korrigiert.

Ich habe auch PR2 geöffnet (Hinzufügen der Neuberechnungen nur für Features, ohne die Datumsbereiche, die in PR3 folgen werden) zum Referenzieren.

Ich habe es noch nicht alles erneut getestet. Das mache ich morgen Abend.

Jedenfalls bin ich auf meiner Ebene ziemlich beeindruckt von der Sorgfalt, mit der Terdious das beschreibt, was er vorbereitet hat. Ich hoffe, ihr findet gemeinsam einen Weg, diese Entwicklung bis zur Produktion zu bringen, denn es wird wirklich nützlich sein, diese gezielten Neuberechnungen durchführen zu können.

Hallo @StephaneB,

Danke für deine Nachricht, das berührt mich.

Ich möchte trotzdem, aus Ehrlichkeit und Transparenz gegenüber Pierre-Gilles, einen Punkt relativieren:

Ich bin kein Berufsentwickler, ich bin Autodidakt. Ich habe klare Ideen und schaffe es normalerweise, das zu codieren, was ich brauche, aber bei der Architektur-Genauigkeit stütze ich mich stark auf die Rückmeldungen in den Reviews, um mich zu verbessern — und es ist völlig normal, dass das Pierre-Gilles mehr Zyklen mit mir erfordert als mit anderen Beitragenden.

Übrigens hatte ich bereits in der PR1 die Entscheidung getroffen, zwei Funktionen zu duplizieren, anstatt die bestehenden zu erweitern — aus übermäßiger Vorsicht beim Core von Gladys, von dem ich weiß, dass er sensibel ist. Das war am Ende die falsche Wahl in diesem Kontext, und Pierre-Gilles hatte recht, mich darauf hinzuweisen. Ich habe es in die richtige Richtung refaktorisiert.

Also ja, ich möchte diese Entwicklung vorantreiben, weil ich glaube, dass sie einem echten gemeinsamen Bedarf dient — aber ohne mir Qualitäten zuzuschreiben, die ich nicht habe. Die Sorgfalt, die ich in die Beschreibung und die Tests stecke, kommt hauptsächlich von der Zeit, die ich investiere, und den Werkzeugen, die ich verwende, nicht von einem Profi-Dev-Niveau.

Danke trotzdem für deine Unterstützung.

Hallo @pierre-gilles,
Nur zur Info, ich habe ein Testergebnis zur PR1 (#2528) gepostet. Ich habe gestern Abend ein Docker-Image terdious/gladys:energy-monitoring-pr1 auf einer isolierten Kopie meiner Produktionsdatenbank laufen lassen, eine vollständige Neuberechnung der Kosten gestartet (1,5 Stunden, saubere Logs) und die neu berechneten Werte monats- und jahresweise mit meiner Produktion verglichen — es passt.

Ich kann es noch ein oder zwei Tage laufen lassen, um die Inkrementierung über die Zeit zu validieren, oder direkt mit einem Test-Image der PR2 weitermachen, wenn du das bevorzugst. Lass es mich wissen.

Vielen Dank, ich sag dir Bescheid, sobald ich es mir angeschaut habe!