-
Notifications
You must be signed in to change notification settings - Fork 29
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
Allow TestScope to be used in multi-threaded tests #2
Conversation
This isn't strictly necessary given how the code works now, but it is more robust.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/. If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits. Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name. |
|
||
/** | ||
* Scope for bindings annotated with {@link TestScoped}. | ||
*/ | ||
class TestScope implements Scope { | ||
private final ThreadLocal<Map<Key<?>, Object>> values = new ThreadLocal<>(); | ||
private AtomicReference<ConcurrentMap<Key<?>, Object>> values = new AtomicReference<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous behavior allowed for test runners which execute multiple tests in parallel. I think that will no longer work with this change.
I added a test in commit 3c39c24 to clarify the existing behavior. I think if a single test case uses multiple threads it may be best for that test just to get the instances it needs on the main test thread and then pass those to the other threads.
What do you think? Is there another option which does what you want while keeping the support for parallel test runners? Can you add a test case which motivates this change to clarify what it enables?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The situation is that I have an in-process server that handles requests on separate threads and uses an in-process database. I issue several requests / test and want separate server state / test.
I didn't think about running tests in parallel. This changes things. For my case, I suppose I would want to either:
- create a separate server/db per test
- use the same server, just use separate dbs / test
- run separate JVMs per test
I'll have to revise my solution (and maybe drop it). InheritableThreadLocal might turn out to be useful, if each test created its own threads, or it might turn out to be just more confusing. I'll go ahead and drop the request for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I've got around this by sharing the same server across all test cases and either wiping the database between tests using a TestingService or sharding the test data (e.g., using a different user for each test, assuming there is no relevant global state).
Using a separate server or JVM per test is often a bit slow.
I wouldn't use @TestScoped
bindings inside the system under test so normally am injecting only on the main test thread by having @Inject
fields in the test class.
No description provided.