adding simple checkpoint to openhtf.util #816
Conversation
| from openhtf.core import phase_descriptor | ||
| from openhtf.core import test_record | ||
|
|
||
| def Checkpoint(checkpoint_name=None): |
There was a problem hiding this comment.
that's an odd name? I'd expect a Checkpoint to save my progress and let me restart a test from a checkpoint (like in a Mario or Sonic game), which is something I know was previously requested by one of the internal teams.
There was a problem hiding this comment.
You have a pretty narrow definition of checkpoint, from a couple video games. :)
https://en.wikipedia.org/wiki/Checkpoint
I'm using it more in the sense of a place where you are stopped and a check is run to verify certain conditions are meet before you are allowed to proceed.
Open to other naming suggestions.
There was a problem hiding this comment.
The name makes sense to me but is a little nonspecific and could be annoying if we end up implementing the other type of checkpoint down the line.
Looks like internally we called this CheckValidOrDie. Another straightforward option that I like is StopTestIfFailed which I think should make things very clear to test operators and others viewing the test results.
There was a problem hiding this comment.
I'm inclined to go with checkpoints.AlreadyFailing(). I have a bit of a pet peeve against function names being too verbose to the point where they are replacing short docstrings.
There was a problem hiding this comment.
Don't forget that phase lists are normally read directly, and you want to make sure that the phase name makes sense. AlreadyFailing doesn't say what the response is to it already failing, is it setting a measurement to True if it's already failing? StopIfFailing isn't any longer (I think it's actually shorter) and is clearer in its meaning
There was a problem hiding this comment.
Yes, that's why we have a docstring and a logging statement.
I'm also still of opinion that checkpoints as a concept should capture the idea of 'stop' if conditions aren't met.
What I'm trying to avoid is this module turning into a bunch of flow control phases. e.g. XifY. I know there are some requests for that, but I'm of the opinion that is less is more. Start with the simplest case and hope that it captures 95% of the the use cases. We can re-examine the remaining 5% ad consider whether they should be added to the framework or are better handled in one-off fashion.
There was a problem hiding this comment.
On the other hand if this phase is already doing flow control, it might be better to admit it than try to hide it. I'm mostly thinking of operators on the line for whom this could be confusing. They don't have docstrings and in general a single log line passes by very quickly and won't be seen (limitations of the UI design).
Anyway I don't like AlreadyFailing but if you prefer Checkpoint to StopIfFailing I'm fine getting this in and seeing how it goes.
| from openhtf.core import phase_descriptor | ||
| from openhtf.core import test_record | ||
|
|
||
| def Checkpoint(checkpoint_name=None): |
There was a problem hiding this comment.
The name makes sense to me but is a little nonspecific and could be annoying if we end up implementing the other type of checkpoint down the line.
Looks like internally we called this CheckValidOrDie. Another straightforward option that I like is StopTestIfFailed which I think should make things very clear to test operators and others viewing the test results.
|
I was going to suggest something along the lines of `StopTestIfFailed`
…On Tue, Aug 28, 2018 at 11:44 AM Kenneth Schiller ***@***.***> wrote:
***@***.**** approved this pull request.
LGTM. If you remember who was interested in using this feature it'd be
nice to have them take a quick look as well
------------------------------
In openhtf/util/checkpoints.py
<#816 (comment)>:
> +phase has a failed measurement. This is desirable behavior in many cases
+because you want to acquire additional information before the test stops.
+
+However, in some cases it's less desirable because downstream long-running
+phases will be executed only to FAIL at test completion due to the failing
+measurements. This can have negative implications for tact time.
+
+Checkpoints allow test authors to have their cake and eat it too. A checkpoint
+checks the result of all previous phases and will stop test execution if a
+previous phase has failed.
+"""
+
+from openhtf.core import phase_descriptor
+from openhtf.core import test_record
+
+def Checkpoint(checkpoint_name=None):
The name makes sense to me but is a little nonspecific and could be
annoying if we end up implementing the other type of checkpoint down the
line.
Looks like internally we called this CheckValidOrDie. Another
straightforward option that I like is StopTestIfFailed which I think
should make things very clear to test operators and others viewing the test
results.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#816 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AASrtJ1hS9x9xdFOquikbKapRywvSwUmks5uVY-agaJpZM4WOsfd>
.
|
This change is