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

Introduce dmsg Tests fs10 with corrupt data #1034

Merged
merged 27 commits into from
Nov 21, 2021
Merged

Introduce dmsg Tests fs10 with corrupt data #1034

merged 27 commits into from
Nov 21, 2021

Conversation

sidey79
Copy link
Contributor

@sidey79 sidey79 commented Nov 16, 2021

  • Please check if the PR fulfills these requirements
  • Tests for the changes have been added / modified (needed for for bug fixes / features)
  • commandref has been added / updated (needed for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
  • Bugfix (please link issue)
  • Feature enhancement
  • Documentation update
  • Unittest enhancement
  • other
  • What is the current behavior?
    (You can also link to an open issue here, if this describes the current behavior)

Coverage decreases for some code parts due to changed regex in #1033

  • What is the new behavior (if this is a feature change)?

FS10 module is tested with currupt data to test some more code paths

There is also a additional test module , which saves repeating steps performing a dmsg test for modules
Reduced test runtime, prove runs now two tests in parallel

  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

no

  • Other information:

sidey79 and others added 4 commits November 16, 2021 23:13
Testtool entworfen, welches eingesetzt werden kann um dmsg Testdaten zu verifizieren
Rückgabewert vom Modul standardkonfom angepasst. undef wenn eine Nachricht nicht verarbeitet werden konnte
Test auf Basis RDMsg aufgebaut und testDaten in separte json  Datei ausgelagert.
@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #1034 (664bef4) into master (f21993f) will increase coverage by 1.00%.
The diff coverage is 82.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
+ Coverage   59.84%   60.85%   +1.00%     
==========================================
  Files         111      118       +7     
  Lines        8851     9009     +158     
  Branches     1394     1411      +17     
==========================================
+ Hits         5297     5482     +185     
+ Misses       2577     2520      -57     
- Partials      977     1007      +30     
Flag Coverage Δ
fhem 51.67% <82.97%> (+1.36%) ⬆️
modules 60.85% <82.97%> (+1.00%) ⬆️
perl 91.66% <ø> (+0.08%) ⬆️
unittests 60.85% <82.97%> (+1.00%) ⬆️

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

Impacted Files Coverage Δ
lib/Test2/SIGNALduino/RDmsg.pm 80.26% <80.26%> (ø)
t/FHEM/10_FS10/09_ParseData.t 94.11% <94.11%> (ø)
FHEM/10_FS10.pm 44.87% <100.00%> (+14.52%) ⬆️
t/FHEM/14_SD_BELL/09_parseDatat.t 80.00% <0.00%> (ø)
t/FHEM/14_BresserTemeo/00_load.t 100.00% <0.00%> (ø)
t/FHEM/14_FLAMINGO/09_parseDatat.t 80.00% <0.00%> (ø)
t/FHEM/10_SD_GT/00_load.t 100.00% <0.00%> (ø)
t/FHEM/14_FLAMINGO/00_load.t 100.00% <0.00%> (ø)
FHEM/lib/SD_Protocols.pm 79.26% <0.00%> (+0.23%) ⬆️
FHEM/00_SIGNALduino.pm 63.81% <0.00%> (+0.84%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f21993f...664bef4. Read the comment docs.

actions-user and others added 8 commits November 16, 2021 22:19
renamed from test::RDmsg
copy libdir to fhem working dir
try two tests parallel
detect current directory of testfile to load json
sidey79 and others added 2 commits November 17, 2021 23:18
added some more data to cover errors
@sidey79 sidey79 marked this pull request as ready for review November 17, 2021 22:20
HomeAutoUser
HomeAutoUser previously approved these changes Nov 18, 2021
FHEM/10_FS10.pm Outdated
@@ -300,13 +300,13 @@ sub Parse {

if ($datastart == $blen || $datastart > 12) { # all bits are 0 || more then 12 bit preamble
Log3 $ioname, 4, "$ioname: FS10_Parse $msg - ERROR message contains too many zeros";
return $EMPTY;
return ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Bitte diese Änderungen rückgängig machen, sonst füllt es den Usern das Log mit solchen Ausgaben:

2021.11.17 10:31:06 3: sduinoUSB0: Unknown code P61#06C2C3805F, help me!
2021.11.17 10:31:07 3: sduinoIP: Unknown code P61#06C2C3804, help me!
2021.11.17 10:34:26 3: sduinoUSB0: Unknown code P61#09C2308286, help me!
2021.11.17 10:37:45 3: sduinoACM: Unknown code P61#7858901E2, help me!
2021.11.17 10:37:45 3: sduinoACM: Unknown code P61#06C2C480F1, help me!
2021.11.17 10:37:46 3: sduinoACM: Unknown code P61#09C2318272, help me!
2021.11.17 10:37:47 3: sduinoIP: Unknown code P61#2708C609, help me!
2021.11.17 10:41:04 3: sduinoACM: Unknown code P61#2309243B88, help me!
2021.11.17 10:41:05 3: sduinoACM: Unknown code P61#184921DC4, help me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, das Problem ist, dass vorher ein Wert zurück gegeben wurde.
ParseFN soll aber nur den Devicenamen liefern, wenn eine Definition zugeordnet wurde.

So sieht der Aufruf in den aus:
my @tfound = &{$modules{$mname}{ParseFn}}($hash,$dmsg);

Anhand der Rückgabe wird entweder nach weiteren Modulen gesucht, die etwas damit anfangen können oder nicht.

Ich habe hier auf undef geändert, weil es keine Definition mit dem Wert von Empty gibt, dieser aber wie eine erfolgreiche Verarbeitung gewertet wird.

Das war zunächst der einfachste und stimmigste Weg für mich.
Selbst wenn wir [NEXT] zurück geben erscheint mir das seltsam.

Über einen Weiteren Parameter im Dispatch aufrufs kann das Verhalten wohl auch beeinflusst werden, aber immer will man es ja wohl nicht abstellen oder?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, das Problem ist, dass vorher ein Wert zurück gegeben wurde. ParseFN soll aber nur den Devicenamen liefern, wenn eine Definition zugeordnet wurde.

Naja, den Devicenamen offensichtlich nur, wenn es erfolgreich geparst wurde, denn es gibt noch eine weitere Variante, die nirgends dokumentiert ist...

So sieht der Aufruf in den aus: my @tfound = &{$modules{$mname}{ParseFn}}($hash,$dmsg);

... einige Zeilen darunter:

  # Special return: Do not notify
  return undef if(!defined($found[0]) || $found[0] eq "");

Ich habe hier auf undef geändert, weil es keine Definition mit dem Wert von Empty gibt, dieser aber wie eine erfolgreiche Verarbeitung gewertet wird.

Ich fand es auch etwas merkwürdig, erst eine Variable $EMPTY zu deklarieren, aber perlcritic empfiehlt es so:

https://metacpan.org/pod/Perl::Critic::Policy::ValuesAndExpressions::ProhibitEmptyQuotes

    $message = '';      #not ok
    $message = "";      #not ok
    $message = "     "; #not ok
	
    $message = q{};     #better
    $message = q{     } #better
	
    $EMPTY = q{};
    $message = $EMPTY;      #best
    $SPACE = q{ };
    $message = $SPACE x 5;  #best

Das war zunächst der einfachste und stimmigste Weg für mich. Selbst wenn wir [NEXT] zurück geben erscheint mir das seltsam.

Das habe ich noch nicht probiert, was dann passiert.

Über einen Weiteren Parameter im Dispatch aufrufs kann das Verhalten wohl auch beeinflusst werden, aber immer will man es ja wohl nicht abstellen oder?

Ich denke, so wie es war, ist die beste Lösung, da auch Rudolf Koenig es in seinen Modulen, wie z.B. 14_CUL_WS.pm, 14_CUL_TX.pm, 10_FS20.pm usw. es so handhabt.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, wieso nicht [NEXT]

Ich werde mal schauen was ich mit $EMPTY machen kann, aber $NEXT wäre meiner Ansicht nicht falscher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ich habe es auf empty zurück geändert

FHEM/10_FS10.pm Outdated
}
$bitData = substr $bitData , $datastart;
$blen = length $bitData;
if ($blen < 30 || $blen > 40) {
Log3 $ioname, 4, "$ioname: FS10_Parse $msg - ERROR message too short or too long ($blen bit) ";
return $EMPTY;
return ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Bitte diese Änderungen rückgängig machen, sonst füllt es den Usern das Log mit solchen Ausgaben:

2021.11.17 10:31:06 3: sduinoUSB0: Unknown code P61#06C2C3805F, help me!
2021.11.17 10:31:07 3: sduinoIP: Unknown code P61#06C2C3804, help me!
2021.11.17 10:34:26 3: sduinoUSB0: Unknown code P61#09C2308286, help me!
2021.11.17 10:37:45 3: sduinoACM: Unknown code P61#7858901E2, help me!
2021.11.17 10:37:45 3: sduinoACM: Unknown code P61#06C2C480F1, help me!
2021.11.17 10:37:46 3: sduinoACM: Unknown code P61#09C2318272, help me!
2021.11.17 10:37:47 3: sduinoIP: Unknown code P61#2708C609, help me!
2021.11.17 10:41:04 3: sduinoACM: Unknown code P61#2309243B88, help me!
2021.11.17 10:41:05 3: sduinoACM: Unknown code P61#184921DC4, help me!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ich habe es wieder angepasst

Copy link
Contributor

Choose a reason for hiding this comment

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

Zwei Stellen müsstest du noch wiederherstellen: Zeile 337 und 341

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ich finde dazu kein Kommentar. Ist denn 309 jetzt so wie es sein soll?

@elektron-bbs
Copy link
Contributor

Evtl. ist mein Einspruch ja untergegangen, deshalb nochmal: #1034 (comment)

Rückgabe fehlerhafter Verarbeitung wieder auf $EMPTY angepasst
Verarbeitung angeasst, so dass "leere" oder "[NEXT]" Rückgaben speziell gewertet werden
@elektron-bbs
Copy link
Contributor

Zeile 303 und 309 sind OK.

FHEM/10_FS10.pm Outdated
@@ -334,11 +334,11 @@ sub Parse {
$sum = (10 - $sum) & 7;
if ($sum != $rsum) {
Log3 $ioname, 4, "$ioname: FS10_Parse $msg - ERROR sum=$sum != rsum=$rsum";
return $EMPTY;
return ;
Copy link
Contributor

Choose a reason for hiding this comment

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

Dieser fehlt noch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, passe ich an. Ich ging davon aus, hier kann es bleiben wie ich es vorgeschlagen habe :)

FHEM/10_FS10.pm Outdated
}
if ($gesErr > 0) {
Log3 $ioname, 4, "$ioname: FS10_Parse $msg - ERROR parity/bit5 $gesErr errors";
return $EMPTY;
return ;
Copy link
Contributor

Choose a reason for hiding this comment

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

und dieser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, passe ich an. Ich ging davon aus, hier kann es bleiben wie ich es vorgeschlagen habe :)

elektron-bbs
elektron-bbs previously approved these changes Nov 21, 2021
@sidey79
Copy link
Contributor Author

sidey79 commented Nov 21, 2021

Ich habe noch Konflikte beseitigt, jetzt sollte es passen

@sidey79 sidey79 merged commit ce81453 into master Nov 21, 2021
@sidey79 sidey79 deleted the dmsgTest branch November 21, 2021 20:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants