Add tests for world modelling tools.

[ibrahiminfinite]

This PR will incrementally add tests for the world modelling tools.

Closes #58

[sea-bass]

Looks good! I see that it runs as part of the automated tests (yay pytest), and only have minor comments.

[ibrahiminfinite]

@sea-bass I have updated the tests, let me know if this is a right direction to proceed in.

[sea-bass]

The class + static method + dependencies approach lets you incrementally build the world so you don't have to build one from scratch at each test, right? If so, that is great.

Some general pointers besides the nitpicky stuff in the comments:

• I assume in this same file we'll do tests for adding locations, objects, and robots right?
• You definitely want to add "remove_X" tests and check that the sizes decrease -- plus in that case you want to check that the attached objects also disappear depending on the rules. For example, if you remove a table with 2 objects on top of it, those should go away. Or if you remove a room with 2 tables, those should go away too.
• Generally, you can assert on more things than just number of entities + name. Like, if you add an entity at a particular pose and with a certain color, you can also check those properties are as expected (where applicable). For the Room entity, not a big deal since it's added directly, but for others it might be useful.
• An example of the above that absolutely could use tests is checking automatically generated names. For example, adding 3 apples without explicitly specifying names will name the objects apple0, apple1, etc.
• We want to also test for invalid input, within reason. For example, adding entities that don't have defined categories (like adding an orange when we only have apples and bananas in our list), and removing entities that don't actually exist in the world.

With something along the guidelines above, I would consider there being a good test suite for the "core" world modeling tools.

That being said, some tests are better than none. Whenever you feel you're at a stopping point, let me know and I'll stop suggesting more things to do and move towards approving whatever you want to contribute.

Thank you!

[ibrahiminfinite]

Thanks for the inputs !
I will work on it accordingly and add tests so that at least all the core world building functionalities are tested well.

The class + static method + dependencies approach lets you incrementally build the world so you don't have to build one from scratch at each test, right?

Yes that's right and I also found it helpful with skipping rest of the tests if a prior test failed.

I assume in this same file we'll do tests for adding locations, objects, and robots right?

Yes, I intent to add the rest of the tests to the same file since it can be built incrementally in the class

[ibrahiminfinite]

You definitely want to add "remove_X" tests and check that the sizes decrease -- plus in that case you want to check that the attached objects also disappear depending on the rules. For example, if you remove a table with 2 objects on top of it, those should go away. Or if you remove a room with 2 tables, those should go away too.

@sea-bass
The current tests have good coverage of the core world functions. Let me know what you think of it.

I tried checking for automatic hierarchical clean up by having a room with a location and an object on that location
room --- > location ---> object.
But removing the room did not delete the locations and subsequently the objects.
And removing a location does not automatically remove the objects contained at that location.

Is this an expected behaviour ? , I did not see any logic implemented in the remove_location ( ) or remove_room ( ) that does the expected cleanup.

[sea-bass]

Is this an expected behaviour ? , I did not see any logic implemented in the remove_location ( ) or remove_room ( ) that does the expected cleanup.

I guess this is not in place, now that I read the code... hah. Maybe I should make a gitissue for this.

EDIT: Made #58

[sea-bass]

This is looking good!

My comments are half useful, half formatting.

... I should really add auto-formatting hooks to this repo soon.

[ibrahiminfinite]

@sea-bass

c221a43 adds cleanup for objects when a location is deleted.
Not sure if it should go in this PR since this branch is a lot of commits behind your main branch.

[sea-bass]

c221a43 adds cleanup for objects when a location is deleted. Not sure if it should go in this PR since this branch is a lot of commits behind your main branch.

Normally I'd also say this can go in a separate PR, but no big deal since you started it.

Just make sure you also add the same for remove_room.

And, you can always rebase on / merge main!

[ibrahiminfinite]

I think we can merge this PR now.
Is there any else you think should go into this ?

[sea-bass]

I mostly see a few typos, and possibly missing dependencies in the new test, so once those are fixed up let's get this in.

Thank you!