Created attachment 290094 [details] [review]
Fix GLib test bindings

Review of attachment 290094 [details] [review]:

Thanks for the patch, but I fail to see from the C api how can you add the owned keyword there. Did you actually try compiling any test program with such changes?

::: vapi/glib-2.0.vapi
@@ +3812,3 @@
 		public static void add_func (string testpath, Callback test_funcvoid);
 #endif
+		public static void add_data_func (string testpath, [CCode (delegate_target_pos = 1.9)] owned TestDataFunc test_funcvoid);

Don't know how can this work, did you actually try it?

@@ +3858,2 @@
 #else
+		public TestCase (string test_name, [CCode (delegate_target_pos = 1.9, type = "void (*) (void)")] owned TestFunc data_setup, [CCode (delegate_target_pos = 1.9, type = "void (*) (void)")] owned TestFunc data_func, [CCode (delegate_target_pos = 1.9, type = "void (*) (void)")] owned TestFunc data_teardown, [CCode (pos = 1.8)] size_t data_size = 0);

This is broken from a Vala view point, and with owned keyword it doesn't even compile. How could you compile with such change?

(In reply to comment #2)
> Review of attachment 290094 [details] [review]:
> 
> Thanks for the patch, but I fail to see from the C api how can you add the
> owned keyword there. Did you actually try compiling any test program with such
> changes?
> 
> ::: vapi/glib-2.0.vapi
> @@ +3812,3 @@
>          public static void add_func (string testpath, Callback test_funcvoid);
>  #endif
> +        public static void add_data_func (string testpath, [CCode
> (delegate_target_pos = 1.9)] owned TestDataFunc test_funcvoid);
> 
> Don't know how can this work, did you actually try it?

I've just stared using vala so could be wrong. I've looked at the generated C code and it made sense, I'll have another check later. The patch changes the TestDataFunc delegate to [CCode (scope = "async")] so vala frees the target data after test_func is run. Without [CCode (scope = "async")] vala was freeing the target straight after calling add_data_func. Without the owned it complained about the async. The c api does not provide a way to free the user_data.

GLib is missing the async annotations at the moment, I've reported that as bug 739724.

> 
> @@ +3858,2 @@
>  #else
> +        public TestCase (string test_name, [CCode (delegate_target_pos = 1.9,
> type = "void (*) (void)")] owned TestFunc data_setup, [CCode
> (delegate_target_pos = 1.9, type = "void (*) (void)")] owned TestFunc
> data_func, [CCode (delegate_target_pos = 1.9, type = "void (*) (void)")] owned
> TestFunc data_teardown, [CCode (pos = 1.8)] size_t data_size = 0);
> 
> This is broken from a Vala view point, and with owned keyword it doesn't even
> compile. How could you compile with such change?

I thought sharing the delegate target was probably broken, I added the owned and async to be consistant with add_data_func. Maybe this binding should be deleted?

Right the scope = "async" doesn't add the destroynotify. Forgot that (I even implemented it) :P
However using scope = "async" on top of the delegate itself is discouraged. Can you please try adding scope = "async" to every single parameter instead?

Yes that TestCase binding should probably be deleted, I didn't try but I don't think it can ever compile.

Alternatively one may use has_target = false for the parameter, and pass the target manually.

Created attachment 290497 [details] [review]
Fix GLib test bindings

Thanks for your comments. I've updated the patch as you suggested.  The
new bindings for TestCase() require the delegate target to be passed
manually which makes them a pain to use. It would be nice to have
something like

public delegate T FixtureSetup<T> ();
public delegate void FixtureTest<T> (T);
public TestCase<T> (FixtureSetup<T> setup, FixtureTest<T> test,
FixtureTest<T> cleanup);

but I don't see an easy way of doing that with the C api.

Created attachment 290498 [details]
Program to test the new gtest bindings

This is the program I used to check the new bindings, it also shows how horrible it is to create a test fixture.

I was more about adding has_target on the parameter not on the delegate itself. So that you could still use a method of a class. I will have to try this approach.

Created attachment 290756 [details] [review]
vala glib TestCase bindings

What about the proposed patch? Only applies to glib > 2.26, I wouldn't care about earlier versions because other usage of TestFunc may break.

Example usage:

public struct Fixture {
  public string? s;

  public void setup (string data) {
	  s = " there";
  }

  public void test (string data) {
	  Test.message(data+s);
  }

  public void cleanup (string data) {
	  s = null;
  }
}

int main (string[] args) {
  Test.init (ref args);
  var m = "hello";
  Test.add_func ("/test1", () => { Test.message ("First Test"); });
  Test.add_data_func ("/test2", () => { Test.message (m); });
  var case = new TestCase<string> ("testcase", sizeof (Fixture), m, Fixture.setup, Fixture.test, Fixture.cleanup);
  var suite = TestSuite.get_root ();
  suite.add (case);
  return Test.run ();
}

Note that sizeof() is incorrect for classes. Use a struct.

(In reply to comment #10)
> Created an attachment (id=290756) [details] [review]
> vala glib TestCase bindings
> 
> What about the proposed patch? Only applies to glib > 2.26, I wouldn't care
> about earlier versions because other usage of TestFunc may break.

It looks nicer to use than my version, it's a shame that one still has to do manual memory management of the fixture but I don't think that can be avoided.

I think the current bindings are ok too. Now I'm understanding them better.
For example is this ok for you?

public class Fixture {
  public string? s;

  public Fixture (string s) {
	  this.s = s;
  }
  
  public void setup () {
  }

  public void test () {
      Test.message(s);
  }

  public void cleanup () {
      s = null;
  }
}

int main (string[] args) {
  Test.init (ref args);
  var m = "hello";
  Test.add_func ("/test1", () => { Test.message ("First Test"); });
  Test.add_data_func ("/test2", () => { Test.message (m); });
  var fix = new Fixture ("s");
  var case = new TestCase ("testcase", fix.setup, fix.test, fix.cleanup);
  var suite = TestSuite.get_root ();
  suite.add (case);
  return Test.run ();
}

(In reply to comment #12)
> I think the current bindings are ok too. Now I'm understanding them better.
> For example is this ok for you?
> 
> public class Fixture {
> 
> ....
>
>   public void cleanup () {
>       s = null;
>   }
> }

I don't like the fact that we have to have a cleanup function to free the resources allocated by the class but there doesn't seem to be a way round that. This example using a class rather than a struct for the fixture is nicer as you get automatic initialization.

> int main (string[] args) {
>   Test.init (ref args);
>   var m = "hello";
>   Test.add_func ("/test1", () => { Test.message ("First Test"); });
>   Test.add_data_func ("/test2", () => { Test.message (m); });
>   var fix = new Fixture ("s");
>   var case = new TestCase ("testcase", fix.setup, fix.test, fix.cleanup);
>   var suite = TestSuite.get_root ();
>   suite.add (case);
>   return Test.run ();
> }

Neither this example nor the one in comment 10 compile for me with your proposed bindings. I like the fact that there is no sizeof (Fixture) in the example above, can we make that work with the bindings?

If the bindings can be adjusted to make this work then I'm happy with that, if we can get rid of manually freeing class resources that would be even better.

My last example should work straight with current vala.

(In reply to comment #14)
> My last example should work straight with current vala.

Ah, indeed it does. I think the bindings are nice enough as it is then. Would it be worth marking the fixture delegates as async?

Yes the async scope is correct.

On a side note, I'd even deprecate such functions and use add_func_full instead.

(In reply to comment #17)
> On a side note, I'd even deprecate such functions and use add_func_full
> instead.

I'm happy to do that if you want, but what is the advantage over the async bindings. add_func_full() only calls the destroy function if the test is called, as far as I can see internally it is just the same as calling add_data_func() with [(Ccode scope="async")] for the delegate.

Created attachment 292820 [details] [review]
Fix GLib test bindings

This is an updated patch with leaves the TestCase bindings
alone. Libgee and Gitg are using the current TestCase bindings.

The delegate passed to add_data_func should have async scope.

The first argument to trap_subprocess can be null.

The flaw with the async scope is when you call Test.run() twice. But since I never saw anybody calling it twice, I think it's ok.

(In reply to comment #20)
> The flaw with the async scope is when you call Test.run() twice. But since I
> never saw anybody calling it twice, I think it's ok.

There is the same problem with add_func_full() as it always calls the destroy function after the test function. Indeed it's hard to see what else it can do. Unless there is a convention that it's only called once there's no way for it to tell which invocation is the the last and it should free the data.

add_func_full() semantics is what glib wants and you have don't to care anymore when it's freed. It's the most correct version of all of those.

Created attachment 346741 [details] [review]
glib-2.0: Mark delegates in Test.add_data_func/add_func() as scope=async

Comment on attachment 346741 [details] [review]
glib-2.0: Mark delegates in Test.add_data_func/add_func() as scope=async

commit b9a349109dfa0bf135b4b97f4aef2ce2f8fc826d
Author: Rico Tzschichholz <ricotz@ubuntu.com>
Date:   Sat Feb 25 23:50:57 2017 +0100

    glib-2.0: Mark delegates in Test.add_data_func/add_func() as scope=async