No type safety around test signatures

[Jameskmonger]

Currently, these two compile fine:

@Test()
public someTestWithParams(someParam: any, otherParam: any) { }

@TestCase(1, 2, 3)
public someTestWithoutParams() { }

It would be good to get compile-time safety around these.

[jamesadarich]

Hey, @Jameskmonger not sure we could get much of use at compile time for this but worth an investigation :)

Maybe the better solution is pretest warning messages?

[jamesadarich]

What do you think of the suggestion above @Jameskmonger?

[Jameskmonger]

I have found a solution to this @jamesrichford but it will need to go into 2.0.0 as it will be breaking change (anyone with badly typed tests will break).

[Jameskmonger]

See https://github.com/Microsoft/TypeScript/issues/1024 and http://stackoverflow.com/questions/43668884/get-signature-of-method-from-method-decorator

Another solution would be to change signature to this:

@TestCase([1, 2, null], "Some test case giving 1, 2, null")

This would be much easier to do.

[jamesadarich]

@Jameskmonger looks good. What about for tests without test cases?

[Jameskmonger]

@Test("Some test without arguments")

[jamesadarich]

@Jameskmonger I think you misunderstood. How would you make it type safe.

We need to keep the description separate otherwise you'll have to repeat it for every test case or have inconsistent decorators.

So given this, how will we say how many arguments the function should take if it doesn't have the TestCase decorator?

[Jameskmonger]

Ah sorry yeah I understand. In hindsight I don't think we'll be able to get type safety around the Test decorator in terms of ensuring that it has no parameters.

Of course, we could always roll them into one and have a separate TestDescription decorator?

@TestDescription("Test without parameters")
@Test()
public shouldDoSomething() { }

@TestDescription("Test with parameters")
@Test(1, 3)
@Test(5, 10)
public shouldDoSomething(a: number, b: number) { }

This way we can ensure that test parameters <-> signature parameters always match up

[Jameskmonger]

Anyway, here is my implementation for TestCase:

interface TestCaseDecoratorImplementation<TProperty> {
  (
    target: object,
    propertyKey: string,
    descriptor?: TypedPropertyDescriptor<TProperty>
  ): void;
}

const TestCaseDecorator = <TArgs, TProperty>(args: TArgs): TestCaseDecoratorImplementation<TProperty> => {
  return (
    target: object,
    propertyKey: string,
    descriptor?: TypedPropertyDescriptor<TProperty>
  ) => { }
}

function TestCase<T1, T2>(args: [T1, T2]): TestCaseDecoratorImplementation<(arg1: T1, arg2: T2) => any>;
function TestCase<T1>(args: [T1]): TestCaseDecoratorImplementation<(arg1: T1) => any>;
function TestCase(args: Array<any>) {
  return TestCaseDecorator(args);
}

class SomeClass {
  @TestCase([ "cool value" ]) // ok
  @TestCase([ 3 ]) // not ok
  public stringTestCase(val: string) {}

  @TestCase([ "ok", { val: 5 } ])
  @TestCase([ false, false ]) // not ok
  public twoValTestCase(first: any, second: { val: number }) {}
}

[Jameskmonger]

By making args optional and returning TestCaseDecoratorImplementation<() => any>, we can enforce type safety around the Test decorator (after rolling them into one)

function Test<T1, T2>(args: [T1, T2]): TestCaseDecoratorImplementation<(arg1: T1, arg2: T2) => any>;
function Test<T1>(args: [T1]): TestCaseDecoratorImplementation<(arg1: T1) => any>;
function Test(args?: Array<any>): TestCaseDecoratorImplementation<() => any> {
  return TestCaseDecorator(args);
}

class SomeClass {
  @Test([ "cool value" ]) // ok
  @Test([ 3 ]) // not ok
  public stringTestCase(val: string) {}

  @Test([ "ok", { val: 5 } ])
  @Test([false, false]) // not ok
  @Test()
  public twoValTestCase(first: any, second: { val: number }) { }

  @Test(5, 10) // not ok
  public noValTestCase() { }
}

[jamesadarich]

Probably even better might be to remove the array from the overloads and use the spread operator on the main function :)

[Jameskmonger]

You can't type a spread operator from what I've seen @jamesrichford

[jamesadarich]

I'll show you what I mean either tomorrow or Monday as I'm away from my PC just now @jameskmonger

[jamesadarich]

@Jameskmonger

interface TestCaseDecoratorImplementation<TProperty> {
  (
    target: object,
    propertyKey: string,
    descriptor?: TypedPropertyDescriptor<TProperty>
  ): void;
}

const TestCaseDecorator = <TArgs, TProperty>(args: TArgs): TestCaseDecoratorImplementation<TProperty> => {
  return (
    target: object,
    propertyKey: string,
    descriptor?: TypedPropertyDescriptor<TProperty>
  ) => { }
}

function Test<T1, T2>(arg1: T1, arg2: T2): TestCaseDecoratorImplementation<(arg1: T1, arg2: T2) => any>;
function Test<T1>(arg1: T1): TestCaseDecoratorImplementation<(arg1: T1) => any>;
function Test(): TestCaseDecoratorImplementation<() => any>;
function Test(...args: Array<any>) {
  return TestCaseDecorator(args);
}

class SomeClass {
  @Test("cool value") // ok
  @Test(3) // not ok
  public stringTestCase(val: string) {}

  @Test("ok", { val: 5 })
  @Test(false, false) // not ok
  public twoValTestCase(first: any, second: { val: number }) {}

  @Test(5, 10) // ok?
  public noValTestCase() { }
}

this is roughly what I meant but interestingly looks like your suggestion for empty doesn't flag up an issue with the last scenario

[jamesadarich]

@Jameskmonger it looks like this declaration doesn't care about extra arguments which is causing the issue here because also the below is OK by the compiler

  @Test("cool value", 42) // ok?  
  public stringTestCase(val: string) {}

[Jameskmonger]

That's expected behaviour @jamesrichford - the rest parameter allows for 0 children:

function Foo(...args: Array<any>) {
	return 0;
}

function Bar(args: Array<any>) {
	return 1;
}

Foo(); // ok
Bar(); // Supplied parameters do not match any signature of call target.

[jamesadarich]

@Jameskmonger the problem isn't the 0 children thing it's the extra children that are allowed. This isn't actually to do with the spread operator as we can see this in two other scenarios

function Test(): TestCaseDecoratorImplementation<() => any>;
function Test<T1>(args: [ T1 ]): TestCaseDecoratorImplementation<(arg1: T1) => any>;
function Test<T1, T2>(args: [ T1, T2 ]): TestCaseDecoratorImplementation<(arg1: T1, arg2: T2) => any>;
function Test<T1, T2>(args?: Array<any>) {
  return TestCaseDecorator(arguments);
}

class SomeClass {
  @Test([ "cool value" ]) // ok
  @Test([ "cool value", 42 ]) // ok?
  @Test([ 3 ]) // not ok
  public stringTestCase(val: string) {}

  @Test([ "ok", { val: 5 } ]) // ok
  @Test([ false, false ]) // not ok
  public twoValTestCase(first: any, second: { val: number }) {}

  @Test([ 5 ]) // ok?
  @Test() // ok
  public noValTestCase() { 

  }
}

function Test(): TestCaseDecoratorImplementation<() => any>;
function Test<T1>(arg1: T1): TestCaseDecoratorImplementation<(arg1: T1) => any>;
function Test<T1, T2>(args: T1, arg2: T2): TestCaseDecoratorImplementation<(arg1: T1, arg2: T2) => any>;
function Test<T1, T2>(arg1?: T1, arg2?: T2) {
  return TestCaseDecorator(arguments);
}

class SomeClass {
  @Test("cool value") // ok
  @Test("cool value", 42) // ok?
  @Test(3) // not ok
  public stringTestCase(val: string) {}

  @Test("ok", { val: 5 }) // ok
  @Test(false, false) // not ok
  public twoValTestCase(first: any, second: { val: number }) {}

  @Test(5) // ok?
  @Test() // ok
  public noValTestCase() { 

  }
}

Not had a chance to look at this a great deal but looks like on the face of it this may be a bug in TypeScript

[jamesadarich]

@Jameskmonger for clarity the issues are marked with the comment // ok?

[jamesadarich]

These compile where they shouldn't

[jamesadarich]

The below is something like what we need to do but not possible as things stand (see https://github.com/Microsoft/TypeScript/issues/2607)

export function TestCase<Target extends { [PropertyKey in keyof Target]: Target[PropertyKey] },
PropertyKey extends keyof Target & string>(...testCaseArguments: Parameters<Target[PropertyKey]>) {
	return (
		target: Target,
		propertyKey: PropertyKey,
		descriptor?: TypedPropertyDescriptor<Target[PropertyKey]>
	) => {