Convert more btests to btest_exit. NFC #14415

sbc100 · 2021-06-08T21:33:02Z

There are still many more to convert.. perhaps we should
be more systematic about it. For now I'm just doing them
in batches by hand to keep the reviews of reasonable size.

aheejin · 2021-06-16T02:31:34Z

tests/test_browser.py

    self.btest('clientside_vertex_arrays_es3.c', reference='gl_triangle.png', args=['-s', 'FULL_ES3=1', '-s', 'USE_GLFW=3', '-lglfw', '-lGLESv2'])

  def test_emscripten_api(self):
-    self.btest('emscripten_api_browser.cpp', '1', args=['-s', 'EXPORTED_FUNCTIONS=_main,_third', '-lSDL'])


Sorry I may not have the sufficient context yet.. In the existing code, some calls to btest have the expected value of 1, while other have 0 or something else, but the new btest_exit doesn't contain any. Does deleting these different expected values not matter then? Also, some existing test cases report different values depending on tests using REPORT_RESULT, but they are all deleted. These values don't matter and can be deleted safely?

btest_exit can detect non-zero exit codes but the default and simplest thing to do is to expect an exit code of zero. What I've done in this change is modified some of the tests to report zero on success and non-zero on failure, at least where it makes sense. As far as I can tell, and this is where you can help me out, I don't think I've modified the semantics of any of the tests. They still report success and failures in the same cases.

For an example of a test where I continue to use the non-zero expectation see test_emscripten_api_infloop which as assert_returncode=7. I can't remember why I didn't change that one in particular, but it shows how btest exit works and how it can be used to assert on non-zero exit codes.

aheejin

I see, the removal of random return and expected values looks good! Still some more question though:

aheejin · 2021-06-16T03:00:35Z

tests/emscripten_api_browser.c

  assert(ratio == ratio2);

-  atexit(never); // should never be called - it is wrong to exit the runtime orderly if we have async calls!
+  atexit(check_exit_ok);


The previous code says this should never be called.. Is this still not supposed to be called? If so it'd be good to make that clear in the comments. If not what changed?

Right. Previously EXIT_RUNTIME was not set the test would never exit. With btest_exit we set EXIT_RUNTIME so that test does exit, but crucially it should not exit when main returns but when the async works is all done. I will add a comment.

aheejin · 2021-06-16T03:01:56Z

tests/browser/test_emscripten_unwind_to_js_event_loop.c

+  emscripten_unwind_to_js_event_loop();
+  printf("This should not be called!\n");
+  // Should not reach here
+  return 99;


Do all these return 99s mean this cannot be reached? Would it be better to make emscripten_unreachable() or something in the test framework?

Yes I think are correct. In some case it means unreachable and something like emscripten_unreachable() would make sense. In other cases we are retuning from main but we want to runtime to continue one and the return value should be ignored. Setting it a non-zero value means that in the case the return value is erronously honored we won't get a false positive test success (exit:0).

aheejin · 2021-06-16T03:10:26Z

tests/emscripten_api_browser_infloop.cpp

      emscripten_cancel_main_loop();
+      exit(x);


Including this, there are several places the order of emscripten_cancel_main_loop and exit has changed.. Why is that?

Previously there was no actual exiting of the program so that order of the reporting and the canceling of the loop didn't matter.

Now we want the program to actually exit using exit().. anything that comes after exit is unreachable since exit() will never return. Also exit() won't really exit() if there is still async work going on (such as a main loop) so we need to cancel the loop before calling exit (for a couple of different reasons).

aheejin · 2021-06-16T03:11:36Z

tests/emscripten_get_device_pixel_ratio.c

+int main() {
+  double devicePixelRatio = emscripten_get_device_pixel_ratio();
+  printf("window.devicePixelRatio = %f.\n", devicePixelRatio);
+  assert(devicePixelRatio > 0);


Why is this necessary? If the assertion fails here the code below becomes moot..

Yeah its kind of redundant, I will simplify.

aheejin · 2021-06-16T03:32:27Z

tests/fetch/example_synchronous_fetch.cpp

-#ifdef REPORT_RESULT
-  REPORT_RESULT(fetch->status);
-#endif
+  assert(fetch->status == 200);


Is the assertion failure the expected way to fail? Other tests use return 1, so...

Assertion failure causes the tests to abort which generates a non-zero return code yes.

aheejin · 2021-06-16T03:37:14Z

tests/pthread/test_pthread_locale.c

-#endif

-  pthread_exit(NULL);
+  assert(main_loc == child_loc);


Why is this necessary? Isn't the return below gonna signal whether the test succeeds or not anyway?

You are right I will just stick to one or the other.

aheejin · 2021-06-16T03:37:44Z

tests/pthread/test_pthread_locale.c

-  REPORT_RESULT(result);
-#endif

-  pthread_exit(NULL);


We don't need this anymore?

Calling pthread_exit from main here is just odd.. it might work but it looks pretty out of place. I don't see why it was ever here.

aheejin · 2021-06-16T03:41:21Z

tests/pthread/test_pthread_run_script.cpp

-  REPORT_RESULT(result);
-#endif
-}
+  assert(result == 1);


Here too, is the assertion failure the expected way of failing?

Yes we can use assert to fail best_exit tests test these days. (I think that is true of btest too but I can't remember for sure).

aheejin · 2021-06-16T03:48:28Z

tests/webgl_sample_query.cpp

-#ifdef REPORT_RESULT
-  REPORT_RESULT(result);
-#endif
+  exit(result);


Is this result expected to be 0 when the test succeeds? The same question for webgl_timer_query.cpp.

One needs to look at the test_browser.py to see what the expected return code of the test is:

def test_webgl_timer_query(self): ... self.btest_exit('webgl_timer_query.cpp', args=cmd)

In this case we see no special assert_returncode param so the expected result here is zero.

Should I change this to?:

assert(result == 0); exit(0);

Or maybe:

exit(result == 0 ? EXIT_SUCCESS : EXIT_FAILURE);

There a several different ways we could express it I guess, I'm not sure which is the most clear. Probably the one with the assert?

Actually I quite like the current version because it keeps this change obvious in that it converted REPORT_RESULT(XXX) directly to exit(XXX). Kind of nice one to one mapping and it means the expectation in the python code stays the same.

sbc100 · 2021-06-16T03:55:15Z

tests/browser/test_emscripten_unwind_to_js_event_loop.c

+  emscripten_unwind_to_js_event_loop();
+  printf("This should not be called!\n");
+  // Should not reach here
+  return 99;


Yes I think are correct. In some case it means unreachable and something like emscripten_unreachable() would make sense. In other cases we are retuning from main but we want to runtime to continue one and the return value should be ignored. Setting it a non-zero value means that in the case the return value is erronously honored we won't get a false positive test success (exit:0).

sbc100 · 2021-06-16T03:56:31Z

tests/emscripten_api_browser.c

  assert(ratio == ratio2);

-  atexit(never); // should never be called - it is wrong to exit the runtime orderly if we have async calls!
+  atexit(check_exit_ok);


Right. Previously EXIT_RUNTIME was not set the test would never exit. With btest_exit we set EXIT_RUNTIME so that test does exit, but crucially it should not exit when main returns but when the async works is all done. I will add a comment.

sbc100 · 2021-06-16T03:59:21Z

tests/emscripten_api_browser_infloop.cpp

      emscripten_cancel_main_loop();
+      exit(x);


Previously there was no actual exiting of the program so that order of the reporting and the canceling of the loop didn't matter.

Now we want the program to actually exit using exit().. anything that comes after exit is unreachable since exit() will never return. Also exit() won't really exit() if there is still async work going on (such as a main loop) so we need to cancel the loop before calling exit (for a couple of different reasons).

sbc100 · 2021-06-16T03:59:51Z

tests/emscripten_get_device_pixel_ratio.c

+int main() {
+  double devicePixelRatio = emscripten_get_device_pixel_ratio();
+  printf("window.devicePixelRatio = %f.\n", devicePixelRatio);
+  assert(devicePixelRatio > 0);


Yeah its kind of redundant, I will simplify.

sbc100 · 2021-06-16T04:00:49Z

tests/fetch/example_synchronous_fetch.cpp

-#ifdef REPORT_RESULT
-  REPORT_RESULT(fetch->status);
-#endif
+  assert(fetch->status == 200);


Assertion failure causes the tests to abort which generates a non-zero return code yes.

sbc100 · 2021-06-16T04:02:44Z

tests/pthread/test_pthread_locale.c

-#endif

-  pthread_exit(NULL);
+  assert(main_loc == child_loc);


You are right I will just stick to one or the other.

sbc100 · 2021-06-16T04:03:35Z

tests/pthread/test_pthread_locale.c

-  REPORT_RESULT(result);
-#endif

-  pthread_exit(NULL);


Calling pthread_exit from main here is just odd.. it might work but it looks pretty out of place. I don't see why it was ever here.

sbc100 · 2021-06-16T04:04:16Z

tests/pthread/test_pthread_run_script.cpp

-  REPORT_RESULT(result);
-#endif
-}
+  assert(result == 1);


Yes we can use assert to fail best_exit tests test these days. (I think that is true of btest too but I can't remember for sure).

sbc100 · 2021-06-16T04:08:59Z

tests/webgl_sample_query.cpp

-#ifdef REPORT_RESULT
-  REPORT_RESULT(result);
-#endif
+  exit(result);


One needs to look at the test_browser.py to see what the expected return code of the test is:

def test_webgl_timer_query(self): ... self.btest_exit('webgl_timer_query.cpp', args=cmd)

In this case we see no special assert_returncode param so the expected result here is zero.

Should I change this to?:

assert(result == 0); exit(0);

Or maybe:

exit(result == 0 ? EXIT_SUCCESS : EXIT_FAILURE);

There a several different ways we could express it I guess, I'm not sure which is the most clear. Probably the one with the assert?

sbc100 · 2021-06-16T04:10:24Z

tests/webgl_sample_query.cpp

-#ifdef REPORT_RESULT
-  REPORT_RESULT(result);
-#endif
+  exit(result);


Actually I quite like the current version because it keeps this change obvious in that it converted REPORT_RESULT(XXX) directly to exit(XXX). Kind of nice one to one mapping and it means the expectation in the python code stays the same.

There are still many more to convert.. perhaps we should be more systematic about it.

aheejin

Thanks for the explanation!

Followup to #14415.

Followup to emscripten-core#14415.

sbc100 force-pushed the more_btest_exit branch 4 times, most recently from 04a61db to 8383737 Compare June 9, 2021 23:33

sbc100 requested review from RReverser, dschuff and kripken and removed request for dschuff June 11, 2021 19:20

sbc100 mentioned this pull request Jun 15, 2021

Improve test_sigalrm. NFC #14451

Merged

sbc100 requested a review from aheejin June 15, 2021 21:00

aheejin reviewed Jun 16, 2021

View reviewed changes

sbc100 commented Jun 16, 2021

View reviewed changes

sbc100 force-pushed the more_btest_exit branch from 8383737 to 85af78f Compare June 16, 2021 04:22

Convert more btests to btest_exit. NFC

3fbcb79

There are still many more to convert.. perhaps we should be more systematic about it.

sbc100 force-pushed the more_btest_exit branch from 85af78f to 3fbcb79 Compare June 16, 2021 05:17

aheejin approved these changes Jun 16, 2021

View reviewed changes

sbc100 merged commit 9dd9c35 into main Jun 16, 2021

sbc100 deleted the more_btest_exit branch June 16, 2021 15:55

sbc100 added a commit that referenced this pull request Jun 19, 2021

Convert more btests to btest_exit. NFC

ad587e6

Followup to #14415.

sbc100 added a commit that referenced this pull request Jun 21, 2021

Convert more btests to btest_exit. NFC

bb85113

Followup to #14415.

sbc100 added a commit that referenced this pull request Aug 10, 2021

Convert more btests to btest_exit. NFC

900dbc3

Followup to #14415.

sbc100 mentioned this pull request Aug 10, 2021

Convert more btests to btest_exit. NFC #14829

Merged

sbc100 added a commit that referenced this pull request Aug 17, 2021

Convert more btests to btest_exit. NFC

5fc2025

Followup to #14415.

sbc100 added a commit that referenced this pull request Nov 17, 2021

Convert more btests to btest_exit. NFC

001ac5d

Followup to #14415.

sbc100 added a commit that referenced this pull request Nov 17, 2021

Convert more btests to btest_exit. NFC

bd72e53

Followup to #14415.

sbc100 added a commit that referenced this pull request Nov 17, 2021

Convert more btests to btest_exit. NFC

5399d56

Followup to #14415.

sbc100 added a commit that referenced this pull request Nov 17, 2021

Convert more btests to btest_exit. NFC

6a36318

Followup to #14415.

sbc100 added a commit that referenced this pull request Nov 19, 2021

Convert more btests to btest_exit. NFC

f037df0

Followup to #14415.

sbc100 added a commit that referenced this pull request Nov 19, 2021

Convert more btests to btest_exit. NFC

412481b

Followup to #14415.

sbc100 added a commit that referenced this pull request Nov 19, 2021

Convert more btests to btest_exit. NFC (#14829)

f0c4614

Followup to #14415.

mmarczell-graphisoft pushed a commit to GRAPHISOFT/emscripten that referenced this pull request Jan 5, 2022

Convert more btests to btest_exit. NFC (emscripten-core#14829)

b477de1

Followup to emscripten-core#14415.

Convert more btests to btest_exit. NFC #14415

Convert more btests to btest_exit. NFC #14415

Uh oh!

Conversation

sbc100 commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aheejin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

sbc100 commented Jun 8, 2021 •

edited

Loading