-
Notifications
You must be signed in to change notification settings - Fork 25
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
#169 added parameter for debug wait timeout. #185
base: develop
Are you sure you want to change the base?
Conversation
Can be set in gradle properties by specifying a value for mlDebugWaitTimeout
@@ -71,8 +73,8 @@ declare private function cover:_task-cancel-safe( | |||
for $breakpoint in dbg:breakpoints($id) | |||
return dbg:clear($id, $breakpoint), | |||
dbg:detach($id), | |||
if (fn:empty(dbg:wait($id, 10))) then | |||
fn:error(xs:QName("FAILED-TO-CANCEL"), "unable to cancel a debugging request") | |||
if (fn:empty(dbg:wait($id, if ($TIMEOUT eq "%%mlDebugWaitTimeout%%") then $DEFAULT_TIMEOUT else $TIMEOUT))) |
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.
Note that this reference will be replaced as well, so it will resolve to e.g. "if ($TIMEOUT eq 8)", and it will in fact equal 8 - which means DEFAULT_TIMEOUT will be used instead.
I think you want to do an fn:contains here on "mlDebugWaitTimeout".
I'm still hesitant though about having this rely on a feature of ml-gradle. How about if this depends on a server field instead?
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.
I'm leaning towards a server field here. In my experience, unit tests typically run against a single-node cluster, so the issue of a server field being set on one host but not another won't matter. In a 3+ node cluster, it's still simple enough to have all requests go to the same host.
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.
Expecting a single node to be used and NOT a load balancer feels brittle.
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.
That's fair. How about if we do %%marklogicUnitTestDebugTimeout%%
? That makes it clear it's specific to marklogic-unit-test (wish that itself had a much shorter, snappier name).
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.
@peetkes Just a note, I'm happy to merge this with the above changes.
I agree with the sentiment about implied topology for unit tests.
There's also a slight performance penalty for using a server field vs a
"build" time injection from ml-gradle. To my knowledge everyone loads
their tests using ml-gradle.
That being said I don't believe I've used a debug wait before, so I don't
have personal experience setting this variable or an issue with the default
timeout.
…On Wed, Sep 6, 2023 at 8:26 AM Mads Hansen ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
marklogic-unit-test-modules/src/main/ml-modules/root/test/test-coverage.xqy
<#185 (comment)>
:
> @@ -71,8 +73,8 @@ declare private function cover:_task-cancel-safe(
for $breakpoint in dbg:breakpoints($id)
return dbg:clear($id, $breakpoint),
dbg:detach($id),
- if (fn:empty(dbg:wait($id, 10))) then
- fn:error(xs:QName("FAILED-TO-CANCEL"), "unable to cancel a debugging request")
+ if (fn:empty(dbg:wait($id, if ($TIMEOUT eq "%%mlDebugWaitTimeout%%") then $DEFAULT_TIMEOUT else $TIMEOUT)))
Expecting a single node to be used and NOT a load balancer feels brittle.
—
Reply to this email directly, view it on GitHub
<#185 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACBA7LUJZ5I3QIL5YLX4C7LXZBTYXANCNFSM6AAAAAA3NCHQUA>
.
You are receiving this because you are subscribed to this thread.Message
ID: <marklogic-community/marklogic-unit-test/pull/185/review/1613209771@
github.com>
|
We could adopt MLUT as an acronym for brevity of MarkLogic Unit Test which
would yield mlutDebugTimout as a variable name.
…On Wed, Sep 6, 2023 at 9:45 AM Rob Rudin ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In
marklogic-unit-test-modules/src/main/ml-modules/root/test/test-coverage.xqy
<#185 (comment)>
:
> @@ -71,8 +73,8 @@ declare private function cover:_task-cancel-safe(
for $breakpoint in dbg:breakpoints($id)
return dbg:clear($id, $breakpoint),
dbg:detach($id),
- if (fn:empty(dbg:wait($id, 10))) then
- fn:error(xs:QName("FAILED-TO-CANCEL"), "unable to cancel a debugging request")
+ if (fn:empty(dbg:wait($id, if ($TIMEOUT eq "%%mlDebugWaitTimeout%%") then $DEFAULT_TIMEOUT else $TIMEOUT)))
That's fair. How about if we do %%marklogicUnitTestDebugTimeout%%? That
makes it clear it's specific to marklogic-unit-test (wish that itself had a
much shorter, snappier name).
—
Reply to this email directly, view it on GitHub
<#185 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACBA7LVVB2BS5TBDCEGDLMTXZB47NANCNFSM6AAAAAA3NCHQUA>
.
You are receiving this because you commented.Message ID:
***@***.***
.com>
|
Can be set in gradle properties by specifying a value for mlDebugWaitTimeout