Closed I3igI3uilder closed 2 months ago
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.
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);
Are you willing to try and get a test working? If so I'd suggest the following thoughts:
PactDurationTest
which already has code to make a pact with a duration and let it expirepacts
member from outside, so this may be the wrong wayaccepted != false
on the logic, not the actual details.PactDurationTest
(shortened to basically fast-forward to the expired state without all the intermediate tests which are already done there)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 acceptedI 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
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.
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.
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.
Thank you. Where do I run make test? I use the build setup for Windows like described in the readme.md
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
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.