Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some unit tests fail occasionally when run alongside other tests #1022

Open
davidomid opened this issue Apr 9, 2019 · 8 comments

Comments

Projects
None yet
3 participants
@davidomid
Copy link
Contributor

commented Apr 9, 2019

I have looked into this already and will create a pull request.

Description

Certain unit tests seem to occasionally pass and occasionally fail, when running all tests together.
These tests are related to:

  • AsyncFunctionExceptionAssertionSpecs
  • ExecutionAssertionSpecs
  • FunctionExceptionAssertionSpecs

Expected behavior:

All unit tests should pass reliably, independent from one another.

Actual behavior:

Occasionally some tests will fail when run together.

Complete minimal example reproducing the issue

Run all of the tests, either using the PowerShell build script or through Visual Studio.

Versions

  • Which version of Fluent Assertions are you using? 5.6.1
@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented Apr 9, 2019

Thanks for looking into this, it has been an annoyance for quite some time.
This might be related to the ITimer approach from #1013
Feel free to post findings in this issue, if you want to discuss any changes before creating a PR.

@davidomid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2019

Thanks very much for pointing me in the direction of that pull request, I hadn't seen that and I think that looks like a good approach :)

Some of the transient test failures are related to methods such as ExecutionTime as depending on how many tests are currently running, and plenty of other factors, the machine running a test might run the method slower than usual and it might be below the threshold. A form of abstraction might help with that, so I might look into those particular failures if and when this other PR is merged.

Some of the other tests are related to timing as well. Things like NotThrowAfter are occasionally failing due to the methods being run slower than usual. I'm still investigating but right now it looks like a quick win is to increase the waitTime on some of these tests from small values (i.e 100ms) to larger values (i.e. 2s), like some of the more reliable tests. We can then do subsequent work to use timer abstraction in a further PR in the near future.

@davidomid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

I'm still investigating this but interestingly it looks like removing the redundant multithreading as described in my other issue is actually helping to remove these transient test errors.

I think a reason for this is that Stopwatch isn't thread safe, yet it's being used in tests which may share instances of it across threads as the action being completed uses the same references to stopwatches. I think there's a combination of different issues at work here though.

Will keep updating this issue when I find more.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented Apr 11, 2019

@davidomid Which tests share the same instance of a StopWatch?

@davidomid

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

@jnyrup my mistake, I looked into it further and it looks like I was wrong about instances being shared.

In general it looks like a lot of factors are at work here, related to performance. If I run the tests on a more powerful machine, they pass 99% of the time. On a slower machine it's rare for me to be able to run them all without at least one failure.

They only seem to fail when run together and it looks like it's only affecting tests which test methods which have precise timings, i.e. ones that compare execution time, or call methods at specific intervals, either using Thread.Sleep or Task.Delay.

We have no performance guarantees when using either method of waiting a specific amount of time, and speed is affected by things like CPU load, process priorities and number of concurrent threads, even from other processes. This does help to explain why the tests only fail when run all together.

Maybe we should look into a similar strategy to what you mentioned previously - mocking the timings for the sake of testing the logic of some of these methods, without actually relying on actual times which vary from machine to machine?

@davidomid

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

@dennisdoomen @jnyrup has this now been resolved? A lot of changes were made recently to improve tests related to timing.

@dennisdoomen

This comment has been minimized.

Copy link
Member

commented Jun 20, 2019

Looking at the build history on AppVeyor, not 100%. But the one that was causing the biggest issues, was resolved.

@jnyrup

This comment has been minimized.

Copy link
Collaborator

commented Jun 20, 2019

To my knowledge the only remaining thing is to abstract timing for ExecutionTime, which is tracked in #1085.
So I'm good with closing this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.