Bank Account: attempt to make flaky test case more reliable

Programming C++
1

Testing for code not to be vulnerable to race conditions is inherently hard. The prior version of the test unfortunately led to a lot of false negatives (ie the test passed about 1/3 of the time even the tested code wasn’t thread safe). With these minimal adjustments the test is much more reliable (although not perfect).

Also fixed a typo in a TC name.

Here is the related pull request: Attempt to make flaky test case more reliable by senarclens · Pull Request #1013 · exercism/cpp · GitHub

2

This PR changes the test named “Can handle concurrent transactions” by reducing the number of threads from 1000 to 10.
Doesn’t that make it less likely that it catches implementations that are not thread-safe?

3

Although there are less threads, each one does a lot more work. One of the problems of this test is that each thread computes only a simple integer addition/subtraction, which is usually so fast that most deposits probably finish before most withdrawals even start. Even more so with a 5ms waiting period between threads. Increasing the amount of work per thread should also increase contention/racing between them. Ideally both the number of threads and the work per thread should increase, though. Checking if the increase in one more than compensates the reduction in the other probably needs testing.

1 Like
4

Thanks, I misread the PR.

5

I spent a lot of time with this exercise recently, to implement it in the Lean track. I found a good solution, but unfortunately it is not directly applicable to C++ due to differences between how languages deal with parallelism (in Lean a task is logical, not physical, and there is an internal thread pool coordinating things).

In any case, if it is of any help, here is the PR: Add bank account by oxe-i · Pull Request #114 · exercism/lean · GitHub

6

Gentlemen, sorry for having kept my explanation a bit short. In fact, I’m just a bit of a bean counter, so here’s my personal reason for the PR:

Without the modification, I see roughly 25% false negatives:

$ failures=0; for i in {1..100}; do if ! ./bank-account &> /dev/null; then ((failures++)); fi; done; echo "Total number of failures: $failures"
Total number of failures: 75

With the PR, the situation is certainly not perfect either, but looks better

$ failures=0; for i in {1..100}; do if ! ./bank-account &> /dev/null; then ((failures++)); fi; done; echo "Total number of failures: $failures"
Total number of failures: 100

That said, there is still a chance that code that does not protect against race conditions doesn’t get caught. At least on my CPU (AMD Ryzen 7 5800U) the results were consistently much better though, hence the PR.
The reported deltas are also much better. With my modification, three runs yielded

with expansion:
  175 == 0
with expansion:
  299 (0x12b) == 0
with expansion:
  524 (0x20c) == 0

Without my modification, three failing runs (out of seven total including 4 false negatives) yielded

with expansion:
  1 == 0
with expansion:
  1 == 0
with expansion:
  3 == 0

Maybe it’s now worth considering the PR again. Any better proposals are more than welcome :)

7

The provided data above clearly shows that the suggested PR improves the test case which really adds value. If you don’t trust the data simply run the tests for yourself with and w/out the suggested change. Can someone please either suggest further improvements or accept the PR?
Thanks, Gerald

@vaeng et al. mind having a look? Thx!