-
Notifications
You must be signed in to change notification settings - Fork 33
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
SD_UT RC Buttons_six #1114
SD_UT RC Buttons_six #1114
Conversation
new remote control Westingshouse with 6 buttons
Codecov Report
@@ Coverage Diff @@
## master #1114 +/- ##
==========================================
+ Coverage 64.35% 65.18% +0.82%
==========================================
Files 132 137 +5
Lines 9643 9711 +68
Branches 1530 1536 +6
==========================================
+ Hits 6206 6330 +124
+ Misses 2244 2174 -70
- Partials 1193 1207 +14
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Mhmm, was soll mir das sagen???
|
Keine Ahnung was er uns mitteilen möchte. Hier #1111 lief es noch durch. |
Der Fehler besagt, dass 30% der veränderten Zeilen nicht durch einen Test verifiziert werden. Also jede 3. in etwa: Hier zu sehen, es geht vornehmlich um die roten: Ich könnte mir vorstellen, dass mit relevanten Testdaten die Abdeckung steigt. |
Heißt das jetzt, das ich mehr Testnachrichten hochladen müsste, oder du mehr Tests einbauen musst, oder das das eigentlich egal ist, oder, oder...? |
Ich habe jetzt nicht nachgesehen, aber es sieht so aus, als ob wir machen Modelle nicht in den Testdaten haben und die sollten wir dort hinterlegen. |
Ich habe nun mal nachgesehen im Modul und im Tool.
Das sind die Modelle, von welchen wir keine Daten im JSON besitzen. Scheinbar ist das codecov/patch noch pingeliger gewurden weil es bis jetzt nichts bemängelt hat grins. Ansonsten kann ich den von @sidey79 obigen markierten Link nicht deuten. |
Im wesentlichen sagt der Link weiter oben, dass die Bedingungen |
Ich finde in meinen Aufzeichnungen (etwa 3,5 Jahre alt) nur diese:
Für Meikee_21 habe ich nichts. Wir haben da nur die Tastencodes abgeglichen, da die Nachrichtenformate identisch sind. Siehe https://forum.fhem.de/index.php/topic,126110.msg1210715.html#msg1210715 |
syntax change return with if statement
Ich habe Tests für die Attribute eingefügt. Ob beim Attribut model alles passt, bin ich mir nicht sicher. |
change bitMSG
…M/RFFHEM into master_SD_UT_Buttons-six
Das Attribut repeats habe ich korrigiert. Da passte das regex nicht. Für das Attribut model habe ich keine Lösung gefunden. Ich dachte erst, das es an der falschen bitMSG liegt, aber das war es offensichtlich nicht:
|
@elektron-bbs |
Ich denke, ich kann es mir schon vorstellen, warum das so nicht funktioniert. |
Die Hypothese hat mir gefallen, aber ich konnte kein umbenanntes device finden. Laut Logfile landen wir auch in Zeile 2346: An diese Stelle sollten wir aber nur gelanden, wenn bitMSG keine Bits entält. Das habe ich verifiziert, bitMSG ist mit Bits gesetzt. Den Fehler habe ich in den Tests gefunden, commandAttr muss einfach noch mal aufgerufen werden und lastMSG wird auch benötigt. |
Commandref mit Wertebereich ergänzt
Mhmm, dann läuft das hier wohl anders ab: Das Attribut model ändern geht nur, wenn vorher eine Nachricht dispatcht wurde.
Jetzt
Dein Test fragt sicher das Attribut model ab, bevor das alte Device gelöscht wird. Zu dem Zeitpunkt existieren noch beide. Aber egal, Hauptsache der Test läuft durch. |
Ich habe die Testdaten wieder hinterlegt. Tests laufen auch, wenn sie durch sind, könnte der Merge heute noch klappen :) |
@elektron-bbs |
Den Branch brauche ich eigentlich nicht mehr.
|
Da ist tatsächlich etwas nicht in Ordnung. |
Vielleicht wäre es ja doch besser, in dem Test 01_attr.t eine Originalnachricht statt der zu kurzen zu verwenden:
Vermeidet dann sicher 4 Warnungen beim Test:
|
Hmm, wäre aber eher ein umgehen des Fehlers, denn da wird wohl nicht geprüft ob lastmsg ausreichend lang für diese Operation ist |
Jetzt, bei genauerem Hinsehen, fällt mir auf, das die Warnungen sich ja auf bitMSG beziehen, im Test wird aber zuerst nur lastMSG gesetzt. lastMSG wird m.E. bei Änderung des Attributes model nicht benötigt. |
@elektron-bbs zur Info. |
Add Testdata for Button six SD_UT SIGNALduino_TOOL#78
What is the current behavior?
(You can also link to an open issue here, if this describes the current behavior)
Remote control is not fully supported
https://forum.fhem.de/index.php/topic,53282.msg1229192.html#msg1229192
What is the new behavior (if this is a feature change)?
Remote control is fully supported
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
no
Other information: