-

2024-11-16 21:41:33

development@hubzilla.org

Harald Eilertsen wrote the following post Sat, 16 Nov 2024 21:41:34 +0100

The Libzot::fetch_conversation function refers to a variable $env['sender'], which does not exist in line 2099. I'm not really sure what to do about that. Just remove it, or is there a need for something more involved to make the function work as it should?

(It would really help if we made tests for new code before adding it! That would uncover stuff like this.)

show all 5 comments

2024-11-16 22:55:27

Harald Eilertsen

harald@hub.volse.no

There's a somewhat similar situation in Module\Tagger: On line 44 it refers to a variable $channel that does not exist. A $channel variable is defined on line 128 though (in the same function), but I'm not sure if that is the expected value that should go in line 44. (It's sadly way to common to redefine variables to contain different data within the same function througout the codebase.)

I've seen a few other instances of variables being used before they were defined, but in those cases it was easier to determine that the values of the later defined variable with the same name, was the same as was expected in where it was being referenced.

2024-11-16 22:59:31

Mario Vavti

mario@hub.somaton.com

The variable can be removed here. I copied this part from import() and missed to remove this one.

How would i test this?

2024-11-16 23:32:20

Mario Vavti

mario@hub.somaton.com

@Harald Eilertsen right, should move $channel definition up in this case.

Thanks for pointing that out!

2024-11-17 08:59:48

Mario Vavti

mario@hub.somaton.com

I have fixed the two issues...

2024-11-17 09:35:12

Harald Eilertsen

harald@hub.volse.no

I copied this part from import() and missed to remove this one.

It's usually better to refactor the code into reusable parts (functions, classes, etc) instead of duplicating it by copying. Makes for way easier maintenance in the future :)

How would i test this?

Start by making a test case that calls the function you want to test. In this case (Libzot::fetch_conversation) you will need to stub some stuff (notably Zotfinger::exec, probably by stubbing some of its dependencies again), and make sure that whatever global state the function may depend on.

Writing tests for a large and complex function like this is painful! But in my opinion that is one of the good things about test driven development: It forces you to factor your code into smaller parts that again is easier to test. Doing so after the fact, is way more work.

But this is also one of the reasons I added PHPStan as a dev dependency. At the very minimum, run it and check for any findings on new code. That should help find this stuff before it gets merged at least.

PHPStan does not replace tests though. The tests will when more complete, double as an executable specification that documents expected behaviour, as well as checking that it actually conforms to it.