Allow to offer a new pact after a pact duration has expired

[I3igI3uilder]

This is my first pull request. Please have mercy :D

GamePlayer::SuggestPact will not allow a request when pact.accepted = true
If you have a pact with a duration that has expired, accepted will never be reset. Just the duration will be set to 0.

I figured this would be the simplest way.

Please let me know if you need more stuff like testing. I am not familiar with that, so I would need more help on that.

A simple test map if it helps...
testpact.zip The AI sends a pact request and (if you accept) will send a new request as soon the previous pact expired.

[Flamefire]

Thanks for the PR, looks good to me. I'd very much like to have a test case reproducing the bug first though. That would be best in https://github.com/Return-To-The-Roots/s25client/blob/master/tests/s25Main/integration/testPacts.cpp where you can simply add a new test based on one of the existing ones reproducing the buggy behavior.

That test should fail without the fix and succeed afterwards. Feel free to push an incomplete solution and ask questions if you are stuck so we can help you.

[I3igI3uilder]

Thank you for answering. I have looked into it for an hour but it is not clear to me how the testing works. I have the following suggestion:

    // pact.accepted must not be true after a pact did expire
    I dont know how to access pacts variable from here or how to set its state
    here I would create a pact or alter the pacts variable to contain a pact that is expired
    this->TestPacts();
    BOOST_TEST_REQUIRE(pacts[curPlayer][PactType::NonAgressionPact].accepted == false);

[Flamefire]

Are you willing to try and get a test working? If so I'd suggest the following thoughts:

• Base your new test on PactDurationTest which already has code to make a pact with a duration and let it expire
• As you cannot (easily) access the pacts member from outside, so this may be the wrong way
• The bug you reported is "GamePlayer::SuggestPact will not allow a request" so testing this makes sense, i.e. the effect of accepted != false on the logic, not the actual details.
• Hence create a pact and let it expire using the code from PactDurationTest (shortened to basically fast-forward to the expired state without all the intermediate tests which are already done there)
• Then call SuggestPact and check the effect of that. E.g. with CheckPactState to check that a request has been created, add a postbox and check that a message arrived (see PactCreatedFixture for an example) and use AcceptPact and then CheckPactState / GetRemainingPactTime / GetPactState to check that the pact can be accepted

I hope that makes sense.

If you are not up to adding a test (I know it is some annoying work) then I can do that, but might be a couple days or so until I got the time. I'll then use your fix on top of that keeping your authorship of course

[I3igI3uilder]

Thank you. I will try to have a look into it once I find the time.

Since I think about adding some functionality around the LUA interface, this task would be a good learning ground.

[I3igI3uilder]

Ok. I think this should more or less be it.
When are these tests executed? During compilation? Compiling did work at least.

Let me know what you think. Thank you for your detailed directions. This gave me a good understanding of how the testing works.

[Flamefire]

When are these tests executed? During compilation? Compiling did work at least.

You need to manually run them. When using make then make test will do that. In Visual Studio there is a "RUN_TESTS" target which you can "build" to run the tests.

Both need to be done after building.

[I3igI3uilder]

Thank you. Where do I run make test? I use the build setup for Windows like described in the readme.md

• running make test in various directories gives me this: make: *** No rule to make target 'test'. Stop.
• BUILD_TESTING is enabled and I see the tests folder appear in the build directory
• I can see RUN_TEST in Visual Studio

[Flamefire]

Thank you. Where do I run make test? I use the build setup for Windows like described in the readme.md

Seems you are not using "make" but Visual Studio so that doesn't apply

* I can see RUN_TEST in Visual Studio

After building everything (I.e. build the "ALL_BUILD" target) "build" that "RUN_TEST". Simply right-click it and click "build" (or something like that)
You can also run a single test by right-clicking it and selecting "debug" or "run" or by right-click -> "set as default target" and F5