Skip to content

idGen tests: Add test to verify parallelism#45

Closed
rmandvikar wants to merge 1 commit intoRobThree:masterfrom
rmandvikar:add-parallel-test
Closed

idGen tests: Add test to verify parallelism#45
rmandvikar wants to merge 1 commit intoRobThree:masterfrom
rmandvikar:add-parallel-test

Conversation

@rmandvikar
Copy link
Copy Markdown
Contributor

@rmandvikar rmandvikar commented Feb 22, 2023

Summary

Add a test to verify things work with parallel calls.

Changes

  • Add a test that verifies things work with parallel calls. This test would catch race conditions, and would have failed due to the bug that was fixed in pr fix: InvalidSystemClockException in concurrent #23.
    For ex, if I regress the above mentioned bug, this test correctly throws IdGen.InvalidSystemClockException.
    System.AggregateException: One or more errors occurred. ---> IdGen.InvalidSystemClockException: Clock moved backwards or wrapped around. Refusing to generate id for 1 ticks
    

Testing

  • Unit Tests
    Passed CreateId_Should_Not_Throw_When_Called_In_Parallel [34 ms]
    

@rmandvikar rmandvikar changed the title idGen tests: Add test for verify parallelism idGen tests: Add test to verify parallelism Feb 22, 2023
@RobThree
Copy link
Copy Markdown
Owner

RobThree commented Feb 22, 2023

There's no guarantee a race condition will occur; whatever the odds (low or high), a unittest should produce deterministic results. Besides that, this slows the unittests down considerably.

That's not to say we shouldn't test for race conditions, I'm currently just short for time.

@RobThree RobThree closed this Feb 22, 2023
@rmandvikar
Copy link
Copy Markdown
Contributor Author

rmandvikar commented Feb 22, 2023

There's no guarantee a race condition will occur; whatever the odds (low or high), a unittest should produce deterministic results.

Sure. But how do you plan to avoid like what happened in pr #23?

Besides that, this slows the unittests down considerably.

The test takes 34 ms to run. How is that slow? It's top 3 in the slowest tests, but it's faster than 2 other tests. IMO, 34 ms is worth it rather than shipping code with bugs, no?

Passed IdGenerator_GetFromConfig_CreatesCorrectGenerator1 [89 ms]
Passed DependencyInjection_Resolves_IdGenerator [83 ms]
Passed CreateId_Should_Not_Throw_When_Called_In_Parallel [34 ms]

That's not to say we shouldn't test for race conditions, I'm currently just short for time.

I'm using this nuget in PRD, so I'm investing time to make patches to it to make it potentially less buggy. What do you propose instead?

@RobThree
Copy link
Copy Markdown
Owner

RobThree commented Feb 22, 2023

Sure. But how do you plan to avoid like what happened in pr #23?

I'd rather have a deterministic unittest than one that fails on uneven tuesdays on a full moon and passes all other days.

What do you propose instead?

Deterministic unittests.

I'm using this nuget in PRD, so I'm investing time to make patches to it to make it potentially less buggy.

You're always welcome to fork and do anything you want with it 😉

All I'm saying is I'm open to better methods of testing for race conditions (like use Concurrency Testing Frameworks or something, code reviews, static code analysis, ...) instead of just doing some arbitrary number of iterations and hope a race occurs (which may differ wildly on machines with less or more cores, more concurrent threads, faster or slower CPU's etc.).

Anyway, your feedback is appreciated, as is you making an effort to improve this library. Let that be clear. I just don't agree on this way of testing.

@rmandvikar
Copy link
Copy Markdown
Contributor Author

rmandvikar commented Feb 23, 2023

Deterministic unittests.

Just curious, how, for the issue at hand?

@RobThree
Copy link
Copy Markdown
Owner

I don't know off-hand. I'll have to sit down and think about that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants