elektron-bbs commented 1 year ago

Please check if the PR fulfills these requirements
[x] Tests for the changes have been added / modified (needed for for bug fixes / features) https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/490abf541c4c9ce005fc17304c8701ed4ca4380d https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/4c8df7704e69f7cf275f64382226adb0f60940d1 https://github.com/RFD-FHEM/SIGNALduino_TOOL/commit/2ca76ef9af5790599c938861ff62be63aa79b834
[x] commandref has been added / updated (needed for bug fixes / features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
[x] Bugfix (please link issue)
[ ] Feature enhancement
[ ] Documentation update
[x] Unittest enhancement
[ ] other
What is the current behavior? (You can also link to an open issue here, if this describes the current behavior) With the Bresser 3-in-1, negative temperatures are calculated positively. Reception frequency of all Bresser protocols (108, 115 and 117) 868.350 MHz. Protocol 115 battery bit wrong.
What is the new behavior (if this is a feature change)? Negative temperatures are calculated correctly with Bresser 3-in-1. Correction of the reception frequency of all Bresser protocols (108, 115 and 117) to 868,300 MHz. Fixed battery bit on protocol 115. Added reading batteryChanged at protocol 115.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) no
Other information:

codecov[bot] commented 1 year ago

Codecov Report

Merging #1125 (db3551e) into master (e9565d6) will increase coverage by 0.42%. The diff coverage is 68.18%.

@@            Coverage Diff             @@
##           master    #1125      +/-   ##
==========================================
+ Coverage   65.31%   65.74%   +0.42%     
==========================================
  Files         139      139              
  Lines        9893     9887       -6     
  Branches     1572     1572              
==========================================
+ Hits         6462     6500      +38     
+ Misses       2175     2129      -46     
- Partials     1256     1258       +2

Flag	Coverage Δ
fhem	`58.04% <68.18%> (+0.49%)`	:arrow_up:
modules	`65.74% <68.18%> (+0.42%)`	:arrow_up:
perl	`90.33% <ø> (+0.16%)`	:arrow_up:
unittests	`65.74% <68.18%> (+0.42%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
FHEM/lib/SD_ProtocolData.pm	`100.00% <ø> (ø)`
lib/Test2/SIGNALduino/RDmsg.pm	`79.31% <40.00%> (-3.05%)`	:arrow_down:
FHEM/14_SD_WS.pm	`68.11% <52.38%> (+3.00%)`	:arrow_up:
t/FHEM/14_SD_WS/09_parseData.t	`94.11% <94.11%> (ø)`
FHEM/00_SIGNALduino.pm	`64.19% <100.00%> (+0.44%)`	:arrow_up:
t/FHEM/14_BresserTemeo/00_load.t
t/FHEM/14_FLAMINGO/09_parseDatat.t
t/FHEM/41_OREGON/09_parseData.t	`80.00% <0.00%> (ø)`
t/FHEM/10_SD_GT/00_load.t	`100.00% <0.00%> (ø)`
... and 2 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

elektron-bbs commented 1 year ago

codecov/patch nervt langsam :-(

Ich habe jetzt noch Testdaten mit negativen Temperaturen für Protokoll 115 hochgeladen, weil das vorhin noch bemängelt wurde. Das Ergebnis ist zwar jetzt etwas besser, aber reicht dem Teil immer noch nicht :-(

sidey79 commented 1 year ago

@elektron-bbs Ja das stimmt, weil für etliche Bedingungen nur ein Fall abgedeckt ist.

Temperaturen ist so ein Fall, ein anderer ist das mit der Feuchtigkeit für bestimmte Wetterstationen und auch kanal. Hast Du denn dazu den Daten?

HomeAutoUser commented 1 year ago

@elektron-bbs Ja das stimmt, weil für etliche Bedingungen nur ein Fall abgedeckt ist.

Temperaturen ist so ein Fall, ein anderer ist das mit der Feuchtigkeit für bestimmte Wetterstationen und auch kanal. Hast Du denn dazu den Daten?

@sidey79 So ganz wird das nie hinhauen. Also sollte man ein Kompromiss eingehen. Deine Argumente, Temperatur, Feuchtigkeit oder Kanal zu prüfen bzw. Tests zu bauen würde verlangen, das wir von allen Sensoren die Datenblätter mit den techn. Details einpflegen. Nur so hättest du plausible Tests.

Desweiteren können wir nicht sicherstellen, das es in Zukunft vielleicht mal Sensoren mit größeren Werten geben könnte. (Bsp: Kanal)
Manche Sensoren laufen im Modul unter dem selben Typ innerhalb des Modules aufgrund des Protokollaufbaues ABER der eine kann nur 3 Kanäle, der andere 5. Wie will man dann plausibel testen?

Schlusswort, auf techn. Details zu prüfen wird nicht komplett machbar sein und man benötigt sehr viel Pflegeaufwand mit Zeit.

elektron-bbs commented 1 year ago

Ich habe jetzt nochmal eine kleine Änderung hochgeladen, um alle Tests auszuführen. codecov/patch mäkelt immer noch, während codecov/project offensichtlich zufrieden ist.

Wenn ich das richtig verstehe, mäkelt codecov/patch hauptsächlich am Modul 14_SD_WS09.pm (rot gekennzeichnet) rum - was hat das mit meinem Pull request / Patch zu tun?

Im Modul 14_SD_WS.pm werden mir jetzt noch Zeilen gelb markiert, die nur unter bestimmten Bedingungen (z.B. if ($rawTemp > 60) ) auftreten. Für alle diese Feinheiten können wir unmöglich jeweils Testdaten liefern.

elektron-bbs commented 1 year ago

Im Modul 14_SD_WS09.pm ist eine offensichtlich ungenutzte sub:

sub SD_WS09_binflip($) {
    my $h = shift;
    my $hlen = length($h);
    my $i = 0;
    my $flip = "";

    for ($i=$hlen-1; $i >= 0; $i--) {
        $flip = $flip.substr($h,$i,1);
    }
    return $flip;
}

Das wären 8 Zeilen die nicht getestet werden können...

sidey79 commented 1 year ago

Interessant, aber das hat mit der Testabdeckung vom Patch nichts zu tun.

Blöde ist ja gerade nur, dass das Versionsupdate an dem Patch Status hängt. Mir ist da die Ziel % Zahl auch nicht transparent, da die auf Auto steht. Was wäre für uns denn ein realistischer Wert? 50% der Änderungen werden in einem Test berücksichtigt?

elektron-bbs commented 1 year ago

Wenn ich die Aussage

42.85% of diff hit (target 56.08%)

richtig interpretiere, werden bei diesem patch jetzt 42,85 % von Tests erfasst. 56,08 % möchte codecov gern getestet haben. Demnach müsstest du als Ziel halt weniger als die 42,85 % einstellen.

Kann es sein, das codecov z.B. bei diesem Beispiel

return if ((substr($rawData,12,1) eq '1' && substr($rawData,33,1) eq '1') || substr($rawData,24,3) !~ m/^\d+$/xms);

dann so rechnet: 3 Bedingungen, macht 8 mögliche Kombinationen. Wenn ich nur eine Kombination teste, sind das dann nur 12,5 %?

sidey79 commented 1 year ago

Wenn ich die Aussage

42.85% of diff hit (target 56.08%) richtig interpretiere, werden bei diesem patch jetzt 42,85 % von Tests erfasst. 56,08 % möchte codecov gern getestet haben. Demnach müsstest du als Ziel halt weniger als die 42,85 % einstellen.

Stimmt, die 56,8% werden halt im Moment dynamisch ermittelt. War schon mal weniger vermute ich.

Kann es sein, das codecov z.B. bei diesem Beispiel

return if ((substr($rawData,12,1) eq '1' && substr($rawData,33,1) eq '1') || substr($rawData,24,3) !~ m/^\d+$/xms);

dann so rechnet: 3 Bedingungen, macht 8 mögliche Kombinationen. Wenn ich nur eine Kombination teste, sind das dann nur 12,5 %?

Ja, genau so wird eine Bedingung berechnet.

Viel Einfacher finde ich aber dieses Beispiel:

$bat = substr($bitData,35,1) eq "0" ? "ok" : "low";

Wenn wir nur den Wert ok haben, dann ist diese Zeile nur zu 50% abgedeckt, sofern Testdaten vorliegen würden wäre das unkompliziert auf 100% zu bringen. Viele andere Stellen sind aber dann wohl nicht so einfach, das leuchtet mir ja durchaus ein.

elektron-bbs commented 1 year ago

Viel Einfacher finde ich aber dieses Beispiel:

$bat = substr($bitData,35,1) eq "0" ? "ok" : "low";

Wenn wir nur den Wert ok haben, dann ist diese Zeile nur zu 50% abgedeckt, sofern Testdaten vorliegen würden wäre das unkompliziert auf 100% zu bringen. Viele andere Stellen sind aber dann wohl nicht so einfach, das leuchtet mir ja durchaus ein.

Tja, ich werde aus der Bewertung von codecov nicht schlau. Die Zeile, die du zitierst markiert es gelb. Sehe ich mir die Auswertung vom Branch master https://app.codecov.io/gh/RFD-FHEM/RFFHEM/blob/master/FHEM/14_SD_WS.pm an, sind ähnliche Batteriezustandserfassungen grün, obwohl wir garantiert in den meisten Fällen auch nur den Zustand "ok" in den Daten haben.

Die zitierte Zeile hat übrigens mit dem Pull request auch nichts zu tun. Die gehört zu Protokoll 33. Die Testdaten für den Sensor TX-EZ6 habe ich vorgestern erst noch hochgeladen, weil mir aufgefallen war, das dieser Abschnitt } elsif (AttrVal($name,'model',0) eq "TX-EZ6") { noch komplett ungetestet war.

sidey79 commented 1 year ago

Stimmt, die Zeile ist nur verrückt. Weiter oben gab es die halt auch noch mal. War aber eh nur zur Bestätigung, wie der Wert errechnet wird.

Also in Einzelfällen könnte man gewisse Bedingungen als nicht testbar markieren:

Z.B. # uncoverable branch true

Oder halt auch nur einzelne Bedingungen # uncoverable condition right

Das würde ich aber nur in den fällen machen wollen, wo es technisch nicht möglich ist. Davon habe ich jetzt aber nicht viele gefunden. Hö höchsten die Prüfung auf eine Zahl vielleicht.

Ich schaue mir das morgen mal an, ansonsten ist mir auch aufgefallen, dass manchmal änderungen in der Testabdeckung erkannt werden wo ich keine erwarten würde.

elektron-bbs commented 1 year ago

Ich habe jetzt den Code von 14_SD_WS.pm noch um 10 Zeilen kürzen können. Diese waren in einem ungetesteten Abschnitt (# Sanity checks).

Das Ergebnis:

vorher:
codecov/patch — 42.85% of diff hit (target 56.08%)
codecov/project Successful in 1s — 55.76% (-0.32%) compared to e9565d6
nachher:
codecov/patch — 37.50% of diff hit (target 56.08%)
codecov/project Successful in 1s — 56.42% (+0.34%) compared to e9565d6

Beim Projekt wird es etwas besser, beim Patch schlechter. Heißt für mich, ich müsste eigentlich ungetesteten Code hinzufügen, damit der Patch durchläuft :-)

Ich hätte einen Vorschlag, wie du diesen relativ großen Abschnitt (# Sanity checks) in einen Test einbeziehen könntest:

Hier sind zwei Datensätze mit großen Sprüngen bei Temperatur und Feuchte:

    {
      "dmsg":"W115#C898BE40041218FF88FF0008017E88FFF04F0000000000000000", "comment":"BRESSER 3-in-1", "user":"elektron-bbs",
      "internals": {"DEF":"SD_WS_115_0", "NAME":"SD_WS_115_0"},
      "readings": {"state":"T: -1.7 H: 88 W: 0.7", "batteryChanged":"1", "batteryState":"ok", "channel":"0", "humidity":"88", "temperature":"-1.7", "type":"Bresser_6in1, new Bresser_5in1", "uv":"0", "windDirectionDegree":"0", "windDirectionText":"N", "windGust":"0.7", "windSpeed":"0.7"},
      "minProtocolVersion":"1.48", "revision_entry":"2022-11-20 19:58:03",
      "revision_modul":"14_SD_WS.pm v3.5.4 2022-11-14 20:37:55Z sidey79",
      "rmsg":"MN;D=C898BE40041218FF88FF0008017E88FFF04F0000000000000000;R=190;"
    },
    {
      "dmsg":"W115#9104143025BE18FFFFFF2928925A97FFF0000000000000000003", "comment":"BRESSER 6-in-1", "user":"elektron-bbs",
      "internals": {"DEF":"SD_WS_115_0", "NAME":"SD_WS_115_0"},
      "readings": {"state":"T: -7.5 H: 97 W: 0", "batteryChanged":"1", "batteryState":"ok", "channel":"0", "humidity":"97", "temperature":"-7.5", "type":"Bresser_6in1, new Bresser_5in1", "uv":"0", "windDirectionDegree":"292", "windDirectionText":"WNW", "windGust":"0", "windSpeed":"0"},
      "minProtocolVersion":"1.48", "revision_entry":"2022-11-20 20:27:05",
      "revision_modul":"14_SD_WS.pm v3.5.4 2022-11-14 20:37:55Z sidey79",
      "rmsg":"MN;D=9104143025BE18FFFFFF2928925A97FFF0000000000000000003;R=189;"
    }

Du müsstest zuerst die erste dmsg übergeben und kurz danach die zweite. Im Log von FHEM sieht das dann so aus:

2022.11.24 17:43:22 4: sduino_dummy: SD_WS_Parse protocol 115, rawData C898BE40041218FF88FF0008017E88FFF04F0000000000000000
2022.11.24 17:43:22 4: sduino_dummy: SD_WS_Parse decoded protocol-id 115 (Bresser_6in1, new Bresser_5in1), sensor-id BE400412
2022.11.24 17:44:10 4: sduino_dummy: SD_WS_Parse protocol 115, rawData 9104143025BE18FFFFFF2928925A97FFF0000000000000000003
2022.11.24 17:44:10 4: sduino_dummy: SD_WS_Parse decoded protocol-id 115 (Bresser_6in1, new Bresser_5in1), sensor-id 143025BE
2022.11.24 17:44:10 3: sduino_dummy: SD_WS_115_0 ERROR - Temp diff too large (old -1.7, new -7.5, diff 5.8)

Danach das Attribut setzen:

attr SD_WS_115_0 max-deviation-temp 50

Jetzt nochmal die zweite Nachricht übergeben, ohne das Device zu löschen. Jetzt wird humidity geprüft, weil wir Temperatur überspringen. Im Log erscheint dann folgendes:

2022.11.24 17:45:48 4: sduino_dummy: SD_WS_Parse protocol 115, rawData 9104143025BE18FFFFFF2928925A97FFF0000000000000000003
2022.11.24 17:45:48 4: sduino_dummy: SD_WS_Parse decoded protocol-id 115 (Bresser_6in1, new Bresser_5in1), sensor-id 143025BE
2022.11.24 17:45:48 3: sduino_dummy: SD_WS_115_0 ERROR - Hum diff too large (old 88, new 97, diff 9.0)

sidey79 commented 1 year ago

Hmm, da müsste ich mir was überlegen um einen Test mit zwei Nachrichten zu machen

sidey79 commented 1 year ago

Also ich habe die Testdaten angepasst. Werte können ja mittels setreadings und Attributes vor dem Aufruf parseFn definiert werden.

So müsste das auch mit der Feuchtigkeit gehen. Der Coverage Bericht ist jetzt aber falsch, da ich meine commits mit force Push aufgeräumt habe. :-(

elektron-bbs commented 1 year ago

Ich blicke bei den Tests nicht durch, was da in welcher Reihenfolge abläuft. Du machst das schon :-)

sidey79 commented 1 year ago

Ich blicke bei den Tests nicht durch, was da in welcher Reihenfolge abläuft. Du machst das schon :-)

Ich musste mich auch erst mal wieder daran erinnern :) Aber es war alles vorberitet. Das setreading und attr wird vor dem Aufruf von parseFn aufgerufen, damit ist es somit kein Problem den Fall zu erzeugen, dass der Temperatur oder Feuchtigkeitssprung zu groß ist.

sidey79 commented 1 year ago

@elektron-bbs

So ich habe die Tests erstellt. Ich habe noch an zwei stellen etwas "optimiert" :)

elektron-bbs commented 1 year ago

Wie bekomme ich die "Unresolved conversations" von "IRadvisor" weg?

sidey79 commented 1 year ago

Wie bekomme ich die "Unresolved conversations" von "IRadvisor" weg?

Ich habe im Moment keine Ahnung, wieso da ein Kommentar ist . Ich sehe es auch nicht

RFD-FHEM / RFFHEM

bugfix Bresser protocols #1125

Codecov Report