search

org-arl / fjage

Framework for Java and Groovy Agents
https://fjage.readthedocs.io/en/latest/
Other
26 stars 13 forks source link

Fix agentsForService test in test_fjage.c #312

Closed brodiealexander closed 3 months ago

brodiealexander commented 3 months ago

Hi Fjåge team!

I'm opening this PR in the hopes of fixing the agentsForService test in test_fjage.c. In the current version, the test submits a null pointer in its call to fjage_agents_for_service in place of the allocated array of pointers it is supposed to submit. I've tested this version against UnetStack by changing the port to 1101 and changing the agent it looks for to be "websh", but I haven't been able to test against vanilla Fjage, as the quickstart script and build directions haven't been working for me.

Best wishes, Brodie

notthetup commented 3 months ago

Thanks @brodiealexander.

brodiealexander commented 3 months ago

I did some additional investigation this morning, and I was wrong about this passing a null pointer. While aid is null here, the pointer to aid created in-line when the call is made is not.

That being said, this format does open the door to testing how the C gateway would respond to multiple agents being returned by agentsForService. I've tested that this works by checking against the list of providers for the DATAGRAM service in a UnetStack container.

Is this something you would be interested in?

As a side note: the fjage_quickstart.sh script began working for me when I swapped out gson-2.8.5 with gson-2.10.1

mchitre commented 3 months ago

As a side note: the fjage_quickstart.sh script began working for me when I swapped out gson-2.8.5 with gson-2.10.1

Thanks for this information. Would you mind opening a PR to update this (and perhaps other dependencies in the quick start script)?

brodiealexander commented 3 months ago

Thanks @brodiealexander.

While the new implementation is more generic, it does not test anything differently from the old implementation, since n = 1. I don't see the point in making the test more complicated to test exactly what it was testing before. On the other hand, if we updated the test to have multiple shell agents and tested that we got multiple agents back, I would be happy to accept that change.

I agree with your assessment here. I might look into this again in the future, but for now I'll close it.

Thanks!