You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "patsonluk (via GitHub)" <gi...@apache.org> on 2023/03/13 22:26:43 UTC
[GitHub] [solr] patsonluk opened a new pull request, #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
patsonluk opened a new pull request, #1457:
URL: https://github.com/apache/solr/pull/1457
https://issues.apache.org/jira/browse/SOLR-16696
# Description
Proposing a new `Breakpoint` feature in `CommonTestInjection` which acts similar to breakpoints in IDE.
This allows blocking/checking conditions at the injection point, which could be useful for race condition emulation and exception verification.
# Solution
Added a new interface `CommonTestInjection$Breakpoint`, with a single method `executeAndResume`. 2 new methods are introduced in `CommonTestInjection`.
`injectBreakpoint(breakpointKey)` should be added in the actual code flow guarded by `assert`, for example `assert injectBreakpoint(MyClass.getClass().getName())`. `assert` will make sure the breakpoint logic would only be evaluated during testing.
The actual breakpoint is set by test cases which calls `setBreakpoint(breakpointKey, breakpointImpl)`, the breakpoint implementation would implement `executeAndResume` method. Example usages:
### Race condition
In the `RaceClass` instance:
```
public void racingMethod() {
//for example a race condition can arise if raceMethod() is invoked and methodA() is called before methodB()
assert CommonTestInjection.injectBreakpoint(RaceClass.class.getName());
methodB();
}
```
in unit test case:
```
CommonTestInjection.setBreakpoint(RaceClass.class.getName(), () -> methodA());
//...
raceClass.racingMethod();
```
### Caught exception verification
In the target class:
```
public void myMethod() {
try {
//...
} catch (SomeException e) {
assert CommonTestInjection.injectBreakpoint("SomeExceptionCheck"); //verify the SomeException was indeed thrown and caught
//do not re-throw the exception
}
}
```
In unit test case:
```
AtomicBoolean exceptionSeen = new AtomicBoolean(false);
CommonTestInjection.setBreakpoint("SomeExceptionCheck", () -> exceptionSeen.set(true))
instanace.myMethod();
assertTrue(exceptionSeen.get())
```
# Tests
No extra tests are added.
# Checklist
Please review the following and check all that apply:
- [x] I have reviewed the guidelines for [How to Contribute](https://wiki.apache.org/solr/HowToContribute) and my code conforms to the standards described there to the best of my ability.
- [x] I have created a Jira issue and added the issue ID to my pull request title.
- [ ] I have given Solr maintainers [access](https://help.github.com/en/articles/allowing-changes-to-a-pull-request-branch-created-from-a-fork) to contribute to my PR branch. (optional but recommended)
- [x] I have developed this patch against the `main` branch.
- [x] I have run `./gradlew check`.
- [ ] I have added tests for my changes.
- [ ] I have added documentation for the [Reference Guide](https://github.com/apache/solr/tree/main/solr/solr-ref-guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on pull request #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #1457:
URL: https://github.com/apache/solr/pull/1457#issuecomment-1716342202
Closing this as the same change is introduced in #1460
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] magibney commented on a diff in pull request #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
Posted by "magibney (via GitHub)" <gi...@apache.org>.
magibney commented on code in PR #1457:
URL: https://github.com/apache/solr/pull/1457#discussion_r1321891475
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,66 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in {@link
+ * CommonTestInjection#injectBreakpoint(String)}
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ */
+ public static boolean injectBreakpoint(String key) {
+ Breakpoint breakpoint = breakpoints.get(key);
+ if (breakpoint != null) {
+ breakpoint.executeAndResume();
+ }
Review Comment:
I wonder if we should add logging here, analogous to what's found in `injectDelay()`? The concept is similar, and we have a ready-made "key" to use in the log message.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on pull request #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on PR #1457:
URL: https://github.com/apache/solr/pull/1457#issuecomment-1714423429
> This looks really useful -- indeed basically _required_ in order to specifically target certain kinds of race/deadlock issues. Basically identical in spirit to `injectDelay()`, but more nuanced and reliable.
>
> I left one minor suggestion, but aside from that, I'm wondering if it might be better to introduce this along with the PR that presumably motivated its addition? I think on usefulness it stands in its own right, but it might be clearer in terms of commit history to bundle this as incorporated with a concrete use case.
Would you mind to review https://github.com/apache/solr/pull/1460 ? :) that PR actually contains this breakpoint change too and uses it.
I have added extra logging when breakpoint is triggered to that branch instead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk closed pull request #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk closed pull request #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
URL: https://github.com/apache/solr/pull/1457
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1457:
URL: https://github.com/apache/solr/pull/1457#discussion_r1321959127
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,66 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in {@link
+ * CommonTestInjection#injectBreakpoint(String)}
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ */
+ public static boolean injectBreakpoint(String key) {
+ Breakpoint breakpoint = breakpoints.get(key);
+ if (breakpoint != null) {
+ breakpoint.executeAndResume();
+ }
Review Comment:
Added with commit https://github.com/apache/solr/pull/1457/commits/53ee6f82098660d86fce5ba8aaabbfe1f5c56341
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
[GitHub] [solr] patsonluk commented on a diff in pull request #1457: SOLR-16696: Breakpoint injection for testing with CommonTestInjection
Posted by "patsonluk (via GitHub)" <gi...@apache.org>.
patsonluk commented on code in PR #1457:
URL: https://github.com/apache/solr/pull/1457#discussion_r1321959127
##########
solr/solrj/src/java/org/apache/solr/common/util/CommonTestInjection.java:
##########
@@ -73,4 +76,66 @@ public static boolean injectDelay() {
}
return true;
}
+
+ /**
+ * This is usually set by the test cases.
+ *
+ * <p>If defined, code execution would break at certain code execution point at the invocation of
+ * injectBreakpoint with matching key until the provided method in the {@link Breakpoint}
+ * implementation is executed.
+ *
+ * <p>Setting the breakpoint to null would remove the breakpoint
+ *
+ * @see CommonTestInjection#injectBreakpoint(String)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should batch the key used in {@link
+ * CommonTestInjection#injectBreakpoint(String)}
+ * @param breakpoint The Breakpoint implementation, null to remove the breakpoint
+ */
+ public static void setBreakpoint(String key, Breakpoint breakpoint) {
+ if (breakpoint != null) {
+ breakpoints.put(key, breakpoint);
+ } else {
+ breakpoints.remove(key);
+ }
+ }
+
+ /**
+ * Injects a breakpoint that pauses the existing code execution, executes the code defined in the
+ * breakpoint implementation and then resumes afterwards. The breakpoint implementation is looked
+ * up by the corresponding key used in {@link CommonTestInjection#setBreakpoint(String,
+ * Breakpoint)}
+ *
+ * <p>An example usages :
+ *
+ * <ol>
+ * <li>Inject a precise wait until a race condition is fulfilled before proceeding with original
+ * code execution
+ * <li>Inject a flag to catch exception statement which handles the exception without
+ * re-throwing. This could verify caught exception does get triggered
+ * </ol>
+ *
+ * <p>This should always be a part of an assert statement (ie assert injectBreakpoint(key)) such
+ * that it will be skipped for normal code execution
+ *
+ * @see CommonTestInjection#setBreakpoint(String, Breakpoint)
+ * @param key could simply be the fully qualified class name or more granular like class name +
+ * other id (such as method name). This should only be set by corresponding unit test cases
+ * with CommonTestInjection#setBreakpoint
+ */
+ public static boolean injectBreakpoint(String key) {
+ Breakpoint breakpoint = breakpoints.get(key);
+ if (breakpoint != null) {
+ breakpoint.executeAndResume();
+ }
Review Comment:
Added with commit https://github.com/apache/solr/pull/1460/commits/356184523b3ed692576502fb90b76e7a7bec7134
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org