Gmxapi stopsignaler test is too strict
Setting a stop signal using gmxapi is done by setting the stophandler signal `stopAtNextNSStep` during force evaluation, and setting the neighborsearching frequency to 1. To test this functionality, the gmxapi stopsignaller test sets the signal during step 0, and checks how many times forces get evaluated in total. It expects that forces are only called twice. This test has two issues.
First, not wrong but confusing, the check is done by comparing
EXPECT_EQ(1, restraint->numberOfTimesCalled()); (src/api/cpp/tests/stopsignaler.cpp:232). The naming of
numberOfTimesCalled() is confusing, as it returns 0 if it has been called once, 1 if it has been called twice, etc. It is effectively returning the last step is has been called (src/api/cpp/tests/stopsignaler.cpp:162). Renaming this function would make the test easier to understand
Second, the test is too strict. It works with the legacy implementation, because the stop signal happens to be checked between the force evaluation and the global communication. It will hence communicate the signal on the same step it is set, and define the next step as the last step. In modular simulator, however, the stop signal gets checked at the beginning of the step. The signal will hence be communicated on the following step, and the simulation stopped two steps after the signal was set. StopSignalHandler is not promising to stop the simulation on the neighbor searching step after it has been set (and hence communicating the signal on the step is has been set), it is only promising to stop the simulation on the neighbor searching step following the step on which it has communicated the signal. The correct test should hence be
Loosen gmxapi stopsignaler test
Gmxapi stopsignaler test was checking whether simulations were stopped
on the step after a stop signal was set. This is true for the legacy
implementation, but only due to the exact order of instructions. The
StopSignalHandler does not guarantee to stop simulations at the next
NS step after a signal has been set, it promises to stop a simulation
at the next NS step after a signal has been communicated. This change
loosens the criterion to reflect this.
- Changes the numberOfTimesCalled() function to timeElapsedSinceStart(),
also including a different treatment of the call before the restraint
is ever called, to avoid misunderstandings.
- Adds a comment to explain the expectations validated by this test.