search

giacomelli / GeneticSharp

GeneticSharp is a fast, extensible, multi-platform and multithreading C# Genetic Algorithm library that simplifies the development of applications using Genetic Algorithms (GAs).
MIT License
1.26k stars 330 forks source link

Speed unit test have asserts inverted #84

Closed jsboige closed 3 years ago

jsboige commented 3 years ago

In Domain Unit tests, linear and parallel speed tests are performed with the comparison asserts in the wrong direction. As a consequence, they just test that the algorithm takes is slower than 1ms.

If "Fast" and "Faster" is what is intended to being measured, the assert param order should be inverted and the target duration set to higher values.

I'm not sure what the expected value should be for linear execution, but with parallel stub fitness and 500ms over 100 generations, I guess a good test would be that the time evolving is no longer than 1 mn (I get 5x seconds with x on the low digits consistently).

giacomelli commented 3 years ago

They are really inverted, the ideal would be to use Assert.Less, because we want to test it if the execution was less than that time.

jsboige commented 3 years ago

OK I did the change in the pull request. For parallel test, I lowered the sleep to 50ms, because I think 50 sec = 500ms*100 was a bit too long for a unit test. The new test is in the 5s+x, and I test for max 10s, which should give some margin with continuous integration.

I also changed the naming for that unit test for "xx_Fast", and created a new "xx_Faster" one, which does compare linear and parallel execution with a fitness sleep of 1 ms. I supposed that was the intended test with that name.

Finally, I noticed continuous control now fails at times on various tests of the TplTaskExecutorTest (see this one or that one for instance). I didn't touch anything to start with there, but I suppose time measurement is tricky with continuous control (I was able to see that for Sudoku tests that would fail only in Appveyor), especially with parallel execution.

I don't know if there is a real issue here with the TplTaskExecutor or even .Net Parallel: a 100ms lag seems like a lot to me, and your tests did seem conservative, but maybe this is to be expected with Appveyor and we should just add some more conservative values?

For now, I only added a comment for the "stop_all" test and the next iteration of Appveyor passed the test, but for some reason, the non blocking test won't pass anymore, even with a more conservative value (EDIT: unless it is unclear, all test have no issues locally, this is just Appveyor exhibiting more difficulties) . I'll leave it there for you to check, but I believe this is outside of the scope of my pull request. The intended evolution speed test do seem to work as expected.

giacomelli commented 3 years ago

It seems to have been some instability in AppVeyor, because now I ran 2 builds for the master and 2 for your pull request and they all passed: image

jsboige commented 3 years ago

OK good to know thanks.