alsatian icon indicating copy to clipboard operation
alsatian copied to clipboard

Alsatian should warn when a test has no Expects executed

Open jhm-ciberman opened this issue 6 years ago • 7 comments

Alsatian should warn when a test has no Expects executed. In my code I have a lots of tests and I found test that werent executed. For example:

	@AsyncTest("should throw when loading an invalid template")
	public async loadFrom_invalid() {
		const loader = new TemplateLoader();
                // Note the commented "return"
		/* return */ loader.loadFrom(this.invalid.dir).then(() => {
			throw new Error("loadFrom() did not throw on invalid json");
		}).catch((e: Error) => {
			Expect(e).toBeDefined();
			Expect(e.message).toContain("Error loading Template");
		});
	}

The asyn test has no returns, so, the Expects are not executed. Alsatian should WARN when a test has no Expects (like other test runners), in this way those typing misstakes could be easily solved.

jhm-ciberman avatar Feb 26 '18 18:02 jhm-ciberman

This would be difficult with the current architecture, I think. It works currently by throwing an exception on failure. We could potentially achieve this by using an emitter pattern rather than relying on exceptions, but then we might lose stack traces...

Jameskmonger avatar Feb 27 '18 15:02 Jameskmonger

I do agree that it would be a useful feature, though, and something we should look to implement.

Jameskmonger avatar Feb 27 '18 15:02 Jameskmonger

From what I can tell, new Error().stack can yield a stack trace without actually throwing the error. Maybe that can help?

Also, I am working on a fluent Expect API for Alsatian, and will look at incorporating this, there. Certain parts of the API already safeguard against incomplete assertions at compile time.

For instance, when asserting on object properties:

Expect(sthg).with.properties({
  Prop0: actual => Expect(actual),
  Prop1: actual => Expect(actual).to,
  Prop2: actual => Expect(actual).to.equal(3)
});

Prop0 and Prop1 above will not compile, because Expect and to return types incompatible with the properties type definition.

However, outside of properties, Expect(sthg).to and Expect(sthg) will not produce compile-time errors (not sure they could), but maybe they could produce run-time errors. I'll think on this a bit, later.

cdibbs avatar Feb 27 '18 17:02 cdibbs

Great suggestion!!!

I think this could tie in with allowing a test to fail multiple times #225

To enable that we'd need to know whether any expect calls failed, perhaps via metadata? As a result we should be able to get this in as well :)

jamesadarich avatar Feb 27 '18 21:02 jamesadarich

With regards to empty expects raised by @cdibbs we should hopefully be able to detect that too if we collect the data about the expects run on that test

jamesadarich avatar Feb 27 '18 21:02 jamesadarich

This could be solved in a similar fashion to my solution on #458

Proof Of Concept:

let expectations = [];

function Expect(anything) {
  expectations.push(anything);

  return { toBe: function() {} };
}

function Test(test) {
  expectations = [];

  test();

  if (expectations.length === 0) {
    console.error(`Test ${test.name} contained no expectations.`);
  }
}

Test(function test1() {
    Expect(1).toBe(1);
});

Test(function test2() {
});

Output:

Test test2 contained no expectations.

pathurs avatar Jul 18 '18 10:07 pathurs

@pathurs absolutely this is roughly what we'd want for multiple failures (#255) all we'd need to do is add them to the metadata but instead of when we call Expect we should do it when a comparison has been made e.g. toBe so we can add that too the list to be executed all at once :)

jamesadarich avatar Jul 18 '18 13:07 jamesadarich