Breakpoint tests #66
Breakpoint tests #66
Conversation
| public SResolver chainedPromise(final VirtualFrame frame, | ||
| final SResolver resolver, final SPromise result, | ||
| @Specialization(guards = {"resolver.getPromise() != resolvedValue"}) | ||
| public SResolver chainedPromise(final VirtualFrame frame, final SResolver resolver, final SPromise resolvedValue, |
There was a problem hiding this comment.
resolvedValue isn't a good name.
This is a promise, so even resolutionValue sounds strange and is unclear later in the code.
| isBreakpointOnPromiseResolution); | ||
| } else if (result.isErroredUnsync()) { | ||
| synchronized (resolvedValue) { | ||
| if (resolvedValue.isResolvedUnsync()) { //resolved |
There was a problem hiding this comment.
resolved what? also the code is pretty clear, it tests whether the 'value' is isResolved.
I still don't like the variable name for resolvedValue or resolutionValue. Would be good if this reads like a promise name.
There was a problem hiding this comment.
what about promiseValue?
| } else { | ||
| synchronized (promise) { // TODO: is this really deadlock free? | ||
| result.addChainedPromise(promise); | ||
| } else { //unresolved promise |
There was a problem hiding this comment.
the test seems still pretty clear to me. I would avoid such trivial comments if possible.
There was a problem hiding this comment.
also, please see the comments around, there is always a space after the //
| @@ -323,7 +325,8 @@ protected static void resolveChainedPromisesUnsync(final SPromise promise, | |||
| // TODO: restore 10000 as parameter in testAsyncDeeplyChainedResolution | |||
| if (promise.chainedPromise != null) { | |||
| Object wrapped = promise.chainedPromise.owner.wrapForUse(result, current, null); | |||
| resolveAndTriggerListenersUnsynced(result, wrapped, promise.chainedPromise, current, isBreakpointOnPromiseResolution); | |||
| // resolveAndTriggerListenersUnsynced(result, wrapped, promise.chainedPromise, current, isBreakpointOnPromiseResolution); | |||
There was a problem hiding this comment.
please don't leave commented code in the files
| @@ -117,6 +117,7 @@ private Breakpoint saveTruffleBasedBreakpoints(final SectionBreakpoint bId) { | |||
| FullSourceCoordinate coord = bId.getCoordinate(); | |||
| BreakpointEnabling<T> existingBP = breakpoints.get(coord); | |||
| if (existingBP == null) { | |||
| WebDebugger.log("SetSectionBreakpoint: " + bId); | |||
There was a problem hiding this comment.
Please don't include debug output in commits.
There was a problem hiding this comment.
in thesaveTruffleBasedBreakpoints method is also used this log, should I remove them both?
There was a problem hiding this comment.
the other possibility would be to change .log() so that it is enabled or disabled via a switch in the ./som executable. but for the moment, it might be easier to just remove them.
smarr
force-pushed
the
breakpoint-tests
branch
from
43a4bd6
to
52e67d2
Compare
- added test for resolver and resolution promise breakpoint - changes in pingpong program to be able to set the breakpoints - add test for promise resolver breakpoint in the case of null resolution - add test for promise resolver breakpoint in the case of chained promises - add test for promise resolution breakpoint in the case of chained promises
- fix promise breakpoint tests - fix line numbers and method names where promise resolver breakpoint is triggered - fix line number and method name where promise resolution breakpoint is triggered for chained resolution
smarr
force-pushed
the
breakpoint-tests
branch
from
52e67d2
to
f27aeb9
Compare
Signed-off-by: Stefan Marr <git@stefan-marr.de>
- renamed and moved old getSectionBreakpointData to be located close to the createLineBreakpointData method Signed-off-by: Stefan Marr <git@stefan-marr.de>
Signed-off-by: Stefan Marr <git@stefan-marr.de>
This fixes #62.