Fail steps/tests/suites when expressions throw exceptions #15
Conversation
docker-compose no longer supported in GHA
The 'expect' assertions are conceptually part of the 'run' success; if 'expect' assertions fail, the 'run' should be retried. We do not need to re-evaluate the 'env' and other keys that are evaluated before the 'run' executes; however, we do need to re-execute the 'run' and re-evaluate the 'expect' assertions.
jonsmock
force-pushed
the
fail-on-expression-exceptions
branch
2 times, most recently
from
fe2b3d0 to
d09caaf
Compare
These test/step execution functions were returning the context for two reasons: * they modified the context's :state :failed boolean (which somewhat conflates the overall run state and the individual test state) * they anticipated supporting test and step outputs in the future However, the :state :failed boolean would then have to be reset to false in 'run-test, if the user was using --continue-on-error. Additionally, returning the context meant we had to restore the previous :env. Instead, it's much simpler to treat the context as immutable! If we need to support outputs in the future, they should be returned as part of the test/step result, and the higher level function can determine how to add those outputs to a newly created context for the next test/step execution.
Extracts 'run-expectations, 'execute-step-retries, 'run-tests, and 'run-suites. The first three of these return a step, step, and suite respectively, so that the bodies of the 'run-test and 'run-suite functions can use them with clojure.core/update. Reorganize logging a bit.
jonsmock
force-pushed
the
fail-on-expression-exceptions
branch
from
d09caaf to
18f8e85
Compare
Expressions in suite/test/step can throw, in which case we want to stop evaluating and fail the suite/test/step (ideally preserving as much information as possible, such as the interpolated :name). This change adds a pending-> macro to handle the common pattern of: 1. check if the suite/test/step is skipped or failed (if so, short circuit) 2. otherwise, attempt to do the next thing (ex: evaluate an env, run a test's steps, etc) 3. if an unexpected exception is thrown, catch it, and mark the suite/test/step as :fail with an error Helpers pass!/skip!/fail! are added to manipulate the suite/test/step outcome consistently and tersely. The pending-> macro has to be extracted into its own namespace and duplicated in both clj and cljs files to support ShadowCLJS and nbb.
Now that suites and tests can throw exceptions, we switch to recording the error on the suite, test, or step as appropriate (previously the step errors were bubbled up and also assoc'd to the test). To support this, we summarize errors at all levels in our summary before displaying the list to the user. As an improvement, we also show where the error came from (previously the user had to infer which step caused the failure).
jonsmock
force-pushed
the
fail-on-expression-exceptions
branch
from
18f8e85 to
2a8c697
Compare
|
Main change from original description:
|
There was a problem hiding this comment.
We could use more docstrings and inline comments. It's pretty dense code with a reasonable level of cyclomatic complexity.
I've marked it approved because I think the doc improvement is general rather than something specific to this change. There is some great information in the commits that should become inline comments/docstrings.
| {:message (str "Exception thrown: " (.-message ~err))})))) | ||
| forms)] | ||
| `(promesa.core/let [~sts ~suite-test-or-step | ||
| ~@(interleave (repeat sts) (butlast forms))] |
There was a problem hiding this comment.
Nice. I understand now what this is doing, but it took some pondering. A comment would be worthwhile.
| (P/let [step (pending-> step | ||
| (->> (skip-if-necessary context)) | ||
| (update :env #(interpolate-any context %))) | ||
| context (update context :env merge |
There was a problem hiding this comment.
The way the context manipulation interrupts the pending-> flow makes me wonder if this points to some higher abstraction. Just a thought.
There was a problem hiding this comment.
Believe me, I've been thinking about that nonstop. If you have even a sketch of a better idea, I'd be willing to try to simplify this more. This was something like my 4th or 5th attempt. I think my ideal would be something where the env doesn't need to be copied into the context, but rather, simply by interpolating it in the suite/test/step, it now participates in future interpolating ... but I couldn't figure out how to make that work that wasn't more complicated than this.
There was a problem hiding this comment.
Yeah, if I come up with something I'll let you know ;-). Maybe after some hammock time.
| @@ -363,14 +408,17 @@ Options: | |||
| (* 1000)))) | |||
|
|
|||
| (defn normalize [suite path] | |||
There was a problem hiding this comment.
A brief docstring summarizing what sort of normalizing we are doing would be good.
|
Thanks, everyone, for the very helpful reviews and feedback. I'll definitely address the docstring/comments in a separate PR. |
This PR conceptually adds try/catch around expressions, failing the step/test/suite if an expression throws an exception. Also, it fixes a bug, where if an
expectcondition fails, therunis not retried even ifrepeatis set correctly.Before we add catching of expression exceptions, there is a pretty substantial refactoring to move the run-(step|test|suite) functions to become "pipelines" using
->andwhile->.while->is a custom macro that takes a predicate as its first argument, then threads the initial value through functions as long as the previous initial or returned value matches the predicate. This is used as an "early exit" mechanism, as we interpolate various parts of steps/tests/suites.Misc:
--continue-on-error