AI units can be auto-sold after dying

[ChrisNeedham24]

In rare circumstances, a deployed AI unit could contribute to an AI player having negative wealth per turn prior to moving, but then make a move and be killed. The unit would subsequently be auto-sold to prevent wealth going below zero, but an exception is thrown because the AI player no longer has the unit.

[Wolfremium13]

Hello @ChrisNeedham24, I want to tackle this issue, but I have a question, Could you tell me how you run the tests? Because I'm having an issue with relative paths, didn't find documentation about set up the tests 😢, this patch throws me FileNotFoundError when I run tests folder on PyCharm:

    @patch("source.saving.game_save_manager.SAVES_DIR", "source/tests/resources")

so I changed to:

    @patch("source.saving.game_save_manager.SAVES_DIR", "resources")

Now It's working, but maybe I need to change something. Then it's not clear to me on which file I should put the new test checking this behavior, because we're calculating and moving on this issue. Thanks for your time 👍🏻

[Wolfremium13]

Hello again!!, for this test, could you give me a more specific clue about the exception? In the other hand, I see you're removing the unit after move_unit function.

[Wolfremium13]

I'm seeing the code, and then I'm thinking on (correct me if I'm wrong):

Scenario "2 AI's playing the game we're on AI 1 turn": Given: "A negative wealth for AI 1" When: "process_ais function, it's processed then calls make_move if unit1 attacks unit2 and unit dies" Then: "AI 1 player wealth check will remove an already removed unit"

Remember, correct me if I'm wrong this is an assumption 👍🏻

[ChrisNeedham24]

Hi @Wolfremium13!

Could you tell me how you run the tests?

Unfortunately I have the same issue sometimes when running the tests on PyCharm as well. However, if you run the tests via the command line, those errors don't occur. It seems that I forgot to put the specific command in CONTRIBUTING.md, but I'll add that in now.

The command is python -m unittest discover -s source/tests -t source/tests.

Then it's not clear to me on which file I should put the new test checking this behavior, because we're calculating and moving on this issue.

Since the change will need to be made in movemaker.py, as that's where the AI move logic occurs, there will also need to be a test added to test_movemaker.py.

for this test, could you give me a more specific clue about the exception?

In the test you linked, no exception is thrown and the test actually tests the business logic path that occurs most of the time. What we are dealing with in this issue is an edge case that only occurs rarely.

In the other hand, I see you're removing the unit after move_unit function.

You've identified exactly where the problem is! The issue is that the moved unit could potentially be killed in the process of the move_unit() function, thus being removed from player.units already. This means that when it is attempted to be removed again, an exception is thrown.

Scenario "2 AI's playing the game we're on AI 1 turn": Given: "A negative wealth for AI 1" When: "process_ais function, it's processed then calls make_move if unit1 attacks unit2 and unit dies" Then: "AI 1 player wealth check will remove an already removed unit"

100% correct. This is the case we need to fix.

[Wolfremium13]

Hello, thank you for your answer, I created a PR with the solution.