jorgenschaefer / emacs-buttercup

Behavior-Driven Emacs Lisp Testing
GNU General Public License v3.0
360 stars 44 forks source link

Allow failing a test immediately if `before-each` fails #189

Open chrahunt opened 3 years ago

chrahunt commented 3 years ago

It often does not make sense to execute tests if there are errors in any of the setup steps. #179 discusses a related issue of ignored errors in before-all. This issue is targeting the expected behavior when an error occurs while executing before-each.

Currently, if an error occurs in a before-each then the corresponding test still executes. To reproduce, see:

script.sh ```sh #!/bin/sh cd "$(mktemp -d)" cat < Cask (source gnu) (source melpa-stable) (development (depends-on "buttercup")) EOF cat < test-example.el (setq i 0) (describe "Running a test" (before-each (print "before-each") (setq i (+ i 1)) (when (> i 1) (expect t :to-be nil))) (before-each (print "before-each 2")) (after-each (print "after-each")) (it "should not execute" (print "1")) (it "should not execute 2" (print "2"))) EOF cask cask exec buttercup -L . ```

which gives this output:

output ``` Loading package information... done Package operations: 1 installs, 0 removals - Installing [ 1/1] buttercup (latest)... done Running 2 specs. Running a test should not execute "before-each" "before-each 2" "1" "after-each" should not execute (0.03ms) should not execute 2 "before-each" "before-each 2" "2" "after-each" should not execute 2 FAILED (4.24ms) ======================================== Running a test should not execute 2 Traceback (most recent call last): (buttercup-fail "%s" "Expected `t' to be `eq' to `nil', but instead it was... (signal buttercup-failed "Expected `t' to be `eq' to `nil', but instead it... FAILED: Expected `t' to be `eq' to `nil', but instead it was `t'. Ran 2 specs, 1 failed, in 5.56ms. ```

The desired behavior is:

  1. No before-each methods execute after the first one fails
  2. The test is not executed
  3. after-each methods are executed

This aligns with the behavior of jasmine when using --stop-on-failure=true (it is false by default). To reproduce, see:

script.sh ```sh #!/bin/sh cd "$(mktemp -d)" npm init -y npm install --save-dev jasmine npx jasmine init cat < spec/test_spec.js var i = 0; describe("Running a test", function() { beforeEach(function() { console.log("beforeEach"); i++; if (i > 1) { throw "Failure"; } }); beforeEach(function() { console.log("beforeEach2"); }); afterEach(function() { console.log("afterEach"); }); it("should not execute", function() { console.log("1"); }); it("should not execute 2", function() { console.log("2"); }); }); EOF npx jasmine --stop-on-failure=true ```

which gives this output:

output ``` Wrote to /tmp/user/1000/tmp.m5b4SFGP8y/package.json: { "name": "tmp.m5b4SFGP8y", "version": "1.0.0", "description": "", "main": "index.js", "scripts": { "test": "echo \"Error: no test specified\" && exit 1" }, "keywords": [], "author": "", "license": "ISC" } npm notice created a lockfile as package-lock.json. You should commit this file. npm WARN tmp.m5b4SFGP8y@1.0.0 No description npm WARN tmp.m5b4SFGP8y@1.0.0 No repository field. + jasmine@3.6.1 added 121 packages from 101 contributors and audited 121 packages in 0.961s found 0 vulnerabilities Randomized with seed 91165 Started beforeEach beforeEach2 1 afterEach .beforeEach afterEach F Failures: 1) Running a test should not execute 2 Message: Failure thrown Stack: 2 specs, 1 failure Finished in 0.005 seconds Randomized with seed 91165 (jasmine --random=true --seed=91165) ```
snogge commented 3 years ago

Thank you for a very thorough issue description.

The problem I see with not running the rest of the before-all, before-each, test code, after-each, after-all is that cleanup code may be skipped. If you have some setup in a before-all and then some necessary cleanup in an after-all, how do you make sure it is executed?

chrahunt commented 3 years ago

I think we should execute the after-each and after-all like normal, even if any before-each fails (item 3 under desired behavior).

snogge commented 3 years ago

Ah, I missed that in your original report. Do you think we should tie this to the same option as #188? If I understand you correctly that is what Jasmine does. (I've never used Jasmine myself, I just know that buttercup is inspired by it.)

chrahunt commented 3 years ago

Jasmine continues executing the remaining tests if a before-each fails and --stop-on-failure=true (and personally this is what I want to happen by default). Only the specific test that the failing before-each preceded is skipped. Here's an updated

script ``` #!/bin/sh cd "$(mktemp -d)" npm init -y npm install --save-dev jasmine npx jasmine init cat < spec/test_spec.js var i = 0; describe("Running a test", function() { beforeEach(function() { console.log("beforeEach"); i++; if (i > 1) { throw "Failure"; } }); beforeEach(function() { console.log("beforeEach2"); }); afterEach(function() { console.log("afterEach"); }); it("should not execute", function() { console.log("1"); }); it("should not execute 2", function() { console.log("2"); }); it("should not execute 3", function() { console.log("3"); }); }); EOF npx jasmine --stop-on-failure=true --random=false ```

and

output ``` Wrote to /tmp/user/1000/tmp.9wT06oYJtr/package.json: { "name": "tmp.9wT06oYJtr", "version": "1.0.0", "description": "", "main": "index.js", "scripts": { "test": "echo \"Error: no test specified\" && exit 1" }, "keywords": [], "author": "", "license": "ISC" } npm notice created a lockfile as package-lock.json. You should commit this file. npm WARN tmp.9wT06oYJtr@1.0.0 No description npm WARN tmp.9wT06oYJtr@1.0.0 No repository field. + jasmine@3.6.1 added 121 packages from 101 contributors and audited 121 packages in 0.91s found 0 vulnerabilities Started beforeEach beforeEach2 1 afterEach .beforeEach afterEach FbeforeEach afterEach F Failures: 1) Running a test should not execute 2 Message: Failure thrown Stack: 2) Running a test should not execute 3 Message: Failure thrown Stack: 3 specs, 2 failures Finished in 0.005 seconds ```

if that makes it a little clearer.

188 sounds closer to the --maxfail/-x option in pytest. I checked the Jasmine docs and I couldn't find an equivalent flag, but we can always take inspiration from more than one place. :smile: