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.

Sign up for GitHub

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

Jump to bottom

Changing instances of ExecutorExtension to static #2088

Merged
merged 1 commit into from
Feb 17, 2022
Merged

Changing instances of ExecutorExtension to static #2088

merged 1 commit into from
Feb 17, 2022

Conversation

gabAlek
Copy link
Contributor

@gabAlek gabAlek commented Feb 11, 2022

Motivation:

To potentially reduce the overhead of instantiating the ExecutorExtension class repeatedly between test cases.

Modifications:
Changed instances of ExecutorExtension to static

Result:

All instances of the ExecutorExtension are now static inside servicetalk-concurrent-api module test classes.

@gabAlek gabAlek mentioned this pull request Feb 11, 2022
Closed
@gabAlek
Copy link
Contributor Author

gabAlek commented Feb 11, 2022

Related to #2074

@bondolo
Copy link
Contributor

bondolo commented Feb 14, 2022

Thank you! In addition to marking the instances static the PR should investigate which of the instances can be made truly static by marking them with setClassLevel(true). At least some of the cases fail with a static executor. I had left the instances as non-static to highlight that they still needed investigation as to whether one instance could be used for all tests. Can you review the tests to see which still work once setClassLevel(true) is applied?

@gabAlek
Copy link
Contributor Author

gabAlek commented Feb 14, 2022 •
edited
Loading

Thank you! In addition to marking the instances static the PR should investigate which of the instances can be made truly static by marking them with setClassLevel(true). At least some of the cases fail with a static executor. I had left the instances as non-static to highlight that they still needed investigation as to whether one instance could be used for all tests. Can you review the tests to see which still work once setClassLevel(true) is applied?

Sure, I'm on it! You're welcome

@gabAlek gabAlek force-pushed the executor-static-instances branch from 15f2e64 to 3b2e2f3 Compare February 15, 2022 01:59
@gabAlek
Copy link
Contributor Author

gabAlek commented Feb 15, 2022

Thank you! In addition to marking the instances static the PR should investigate which of the instances can be made truly static by marking them with setClassLevel(true). At least some of the cases fail with a static executor. I had left the instances as non-static to highlight that they still needed investigation as to whether one instance could be used for all tests. Can you review the tests to see which still work once setClassLevel(true) is applied?

Commit updated

@Scottmitch Scottmitch requested a review from bondolo February 15, 2022 07:14
idelpivnitskiy
idelpivnitskiy reviewed Feb 15, 2022
View reviewed changes
Copy link
Member

@idelpivnitskiy idelpivnitskiy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR @gabAlek!
Please check build failures, there are some violations of checkstyle rules. You can test it locally with ./gradlew checkstyle or ./gradlew build before pushing an update.

...icetalk-concurrent-api/src/test/java/io/servicetalk/concurrent/api/BufferStrategiesTest.java Outdated
@@ -80,7 +80,7 @@
static final ExecutorExtension<Executor> EXECUTOR_EXTENSION = withCachedExecutor().setClassLevel(true);

@RegisterExtension
final ExecutorExtension<TestExecutor> testExecutorExtension = withTestExecutor();
static final ExecutorExtension<TestExecutor> testExecutorExtension = withTestExecutor();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can not use .setClassLevel(true) in this case or any other, it's better to keep it as non-static variable. Otherwise, it's harder to understand the test flow and when this extension is re-initialized.

@gabAlek gabAlek force-pushed the executor-static-instances branch from 3b2e2f3 to 29eabfd Compare February 16, 2022 02:35
@gabAlek
Copy link
Contributor Author

gabAlek commented Feb 16, 2022

Thank you for the PR @gabAlek! Please check build failures, there are some violations of checkstyle rules. You can test it locally with ./gradlew checkstyle or ./gradlew build before pushing an update.

Commit updated!Thank you for the suggestions, @idelpivnitskiy

bondolo
bondolo reviewed Feb 16, 2022
View reviewed changes
Copy link
Contributor

@bondolo bondolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work! This is getting very close to completion. Thank you!

...-api/src/test/java/io/servicetalk/concurrent/api/completable/CompletableToPublisherTest.java Outdated Show resolved Hide resolved
...ent-api/src/test/java/io/servicetalk/concurrent/api/completable/CompletableToSingleTest.java Outdated Show resolved Hide resolved
...concurrent-api/src/test/java/io/servicetalk/concurrent/api/single/SingleToPublisherTest.java Outdated Show resolved Hide resolved
@bondolo bondolo added technical debt Reduces technical debt new contributor PR by a new contributor labels Feb 16, 2022
Changing instances of ExecutorExtension to static
d82d260 
Motivation:

To potentially reduce the overhead of instantiating the ExecutorExtension class repeatedly between test cases.

Modifications:
Changed instances of ExecutorExtension to static, except for classes where said instances were created with a TestExecutor (which caused unit tests to break).

Result:

All instances of the ExecutorExtension are now static inside servicetalk-concurrent-api module test classes. Instances that were not created with TestExecutor were set to class level.
@gabAlek gabAlek force-pushed the executor-static-instances branch from 29eabfd to d82d260 Compare February 17, 2022 00:46
@bondolo bondolo merged commit a3fe3fa into apple:main Feb 17, 2022
@bondolo
Copy link
Contributor

bondolo commented Feb 17, 2022

Thank you Gabriel!

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

@idelpivnitskiy idelpivnitskiy idelpivnitskiy left review comments

@bondolo bondolo bondolo approved these changes

Assignees

@bondolo bondolo

Labels
new contributor PR by a new contributor technical debt Reduces technical debt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gabAlek @bondolo @idelpivnitskiy