Option for failing the entire test when a step fails.

[jespertheend]

Maybe I'm misunderstanding how test steps are supposed to be used, but the way I'm using them is to describe steps that are taken during e2e tests with puppeteer. So that, should a test fail, it's immediately clear where to look for the failure.

But at the moment, if a step fails, Deno happily continues executing the other steps. Which is rather pointless in this specific case, because if a step fails, the other steps will likely also fail. In the case of puppeteer this means lots of timeouts, so every step takes 30 seconds and then fails, causing the full test to take a very long time.

Would it be possible to add an option to TestStepDefinition to indicate that the full test should fail if the step fails. Or maybe even a somewhat more global option, since in my case you'll want to use this for every step anyway.

I tried creating a wrapper function that does this, but if you have many nested steps this causes each step to throw an error individually. Causing the test results to contain lots of similar stack traces even though the first error is the only useful one.

Here's the wrapper function:

import {assert} from "[std/testing/asserts](https://deno.land/[email protected]/testing/asserts.ts)";
export async function stepAndAssert(context, testStepDefinition) {
	const result = await context.step(testStepDefinition);
	assert(result, "Test step failed.");
}

[bartlomieju]

Are you looking for --fail-fast flag?

[jespertheend]

Ah good idea, that would sort of work, but I'd like the unit test suite to still run because those tests might give further insights in what is going wrong. So I'd only like the steps from the failing root test to not get executed. Also it seems like --fail-fast doesn't actually block steps from running right now.

Here's an example:

Deno.test("the first test", async (ctx) => {
    await ctx.step("step 1", () => {
        throw new Error("oh no");
    })

    // This step should not be executed
    await ctx.step("step 2", () => {
        throw new Error("not again");
    })
});

// This test should still run
Deno.test("the second test", () => {})

When run, the test results contain stack traces from both new Error("oh no") and new Error("not again"). But I'd like to only see the first error. "the second test" should still run.

The errors section also only shows Error: 2 test steps failed. It would be nice if this showed the first stack trace of the two as well. Though my main concern is that if step 2 takes a long time to run, and step 1 fails, ideally step 2 would just not run at all.

[bartlomieju]

@dsherret please take a look

[lucacasonato]

@jespertheend ctx.step() returns a Promise<bool>, which will be false if the step failed. You can use this to conditionally start the next step only if the first one succeeded.

[jespertheend]

You can use this to conditionally start the next step only if the first one succeeded.

That's what I'm doing in the wrapper function in the first comment :)

The problem with that is that if you have tons of nested steps you'll get tons of similar errors, even though the first one is the only useful one.

[lucacasonato]

Instead of assert, why don't you do if (!result) return? That has the same effect, without the annoying error messages.

Edit: this would not work with nested tests if you wanted to cascade the "fail fast"

[jespertheend]

Yeah exactly, a wrapper function wouldn't be possible and you'd have to wrap every step in a separate if statement. Also the nice thing about asserting is that you can still have cleanup code in a try {} finally {} block.

[lucacasonato]

Also the nice thing about asserting is that you can still have cleanup code in a try {} finally {} block.

You can do that with return too :)

try {
  console.log("1");
  return 2;
} finally {
  console.log("3");
}

[jespertheend]

Oh hah, nice!

Though to add some more context. In the e2e tests I have some helper functions that perform common tasks inside a separate step. Something like:

async function clickGameStart(testContext, page) {
    await testContext.step("Click game start", () => {
        // do something with page
    });
}

So to make this work you'd have to pass the result from testContext.step on to the clickGameStart caller, which can then prevent other code from being run. I was thinking an option to testContext.step to make it reject rather than return false would be more elegant, as it would simply propagate to the root test. This has a nice bonus that the final list of test results only contains the error that triggered the whole chain reaction, rather than a generic Error: 1 test step failed error.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[jespertheend]

I'd like to have a go at this.

[dsherret]

I'm not sure anything needs to change here in Deno. Instead of a wrapper function, a wrapper could be made around the context object so it fails fast.

Deno.test("parent", async (originalCtx) => {
  const t = createFailFastWrapper(originalCtx);

  await t.step("sub test", async t => {
    await t.step("sub sub test 1", () => { throw new Error("FAIL"); });

    await t.step("sub sub test 2", () => { console.log("Wouldn't be run because the previous test fails."); });
  });
});

[jespertheend]

Ah I see, I thought this would require the wrapper to be added to every step, but I realise now that the wrapper could simply add itself to sub steps.

[jespertheend]

I couldn't help myself and tried to see what an implementation in Deno would look like: https://github.com/denoland/deno/pull/15285

I remember now the problem with a wrapper is that the thrown error isn't exposed so you'll end up with several generic error messages that are pretty unhelpful because not even their stack trace contains any reference to the the original error. Unless I'm missing something, I don't think a wrapper can solve this without entirely reimplementing the Deno test step runner.

The changes needed to Deno for this seem pretty minimal (unless I'm missing something significant). But if you prefer to keep the test api as simple as possible, I can understand that.

[dsherret]

Essentially in the wrapper object you don't throw, but you do:

// very roughly and you'd want an actual implementation to be more complex than this
function createFailFastWrapper(originalCtx) {
  let hadFailure = false;
  return {
    async step(action) {
      if (hadFailure) return;
      const result = await originalCtx.step(async (t) => {
        await action(createFailFastWrapper(t));
      });
      if (!result) {
        hadFailure = true;
      }
    }
  };
}

[jespertheend]

Ah I see, I guess I was too focused on propagating the error xD The only difference then, is that the final errors section doesn't contain the actual error, but I can live with that.

Wrapper: throwOnFailure:

[nayeemrmn]

@jespertheend Why not just do

Deno.test("the first test", async (ctx) => {
  assert(await ctx.step("step 1", () => {
    throw new Error("oh no");
  }));

  // This step should not be executed
  assert(await ctx.step("step 2", () => {
    throw new Error("not again");
  }));
});

I don't think you need a wrapper or a new option

Edit: sorry this was discussed above. I still don't get how this is worse/different than the proposed option though

[jespertheend]

When using assert in heavily nested steps, the output looks like this:

The proposed option propagates the option down to subtests, and ensures the thrown error automatically propagates up to the root test. This also allows you to get access to the thrown error. Although to be honest, I can't think of any use cases for that.

[nayeemrmn]

When using assert in heavily nested steps, the output looks like this:

I believe with throwOnError, you'll get the same error printed again for step level. Have you checked otherwise?

The real problem may be that we need to do a more thorough job of excluding useless stack frames.

[jespertheend]

With throwOnError only the initial error is logged, as can be seen in the screenshot from the PR:

Removing useless stack frames would certainly help too, I'm not sure why these are still included because I don't think these are coming from floating promises.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[melbourne2991]

👍

I think the API naming is a little confusing, a "step" (at least to me) implies a sequence of which each item is dependent on the previous.

"unit" may have been closer to the intended abstraction, but it's probably a little late for that now.

Would this be the recommended solution?

Deno.test("sample", async (t) => {
  let proceed = true;

  proceed = await t.step({
    name: "1",
    ignore: !proceed,
    fn: () => {
      throw new Error();
    }
  })
  
  proceed = await t.step({
    name: "2",
    ignore: !proceed,
    fn: () => {
      assert(true)
    }
  })
})

On a semi-related note, I didn't actually realize this^ would be possible until reading this thread because of the docs:

  // steps return a value saying if they ran or not
  const testRan = await t.step({
    name: "copy books",
    fn: () => {
      // ...etc...
    },
    ignore: true, // was ignored, so will return `false`
  });

This seems to imply it would return true if the test ran but failed. In practice it returns true if the test runs and passes. Happy to submit a PR to clarify some of this in the docs if there's appetite for it.