search

dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.08k stars 1.56k forks source link

Some tests use async/await but don't tell the test runner correctly #31535

Open munificent opened 6 years ago

munificent commented 6 years ago

@vsm noticed a few tests that are using async/await but don't use any of the necessary setup work to tell the test runner that the test actually is asynchronous. For example, they use minitest.dart, which is not async-aware. Or they use expect.dart, but don't also use async_helper.dart to tell it when the asynchrony is done.

That means these tests may be erroneously passing because the runner doesn't know to wait until the test is done.

That includes (but is not limited to):

There are a couple of ways to fix these (and any others in the same state):

  1. Switch to using unittest and ensure all async code and expectations live inside functions passed to group() or test() calls. This is usually the easiest change because unittest has a nice API for asynchrony. But, long term, we want to move off unittest since it's deprecated.

  2. Switch to expect or minitest and also use async_helper. The latter has a little manual API you can use to tell the test runner when the async operations are done.

lrhn commented 6 years ago

For language tests, do use async_helper. It uses a minimum of language functionality so you are not using the thing you are testing. I'll fix await_test.

A quick grep for Dart 2 test files that import package:expect and dart:async, but not package:async_helper:

tests/compiler/dart2js/bad_output_io_test.dart
tests/compiler/dart2js_extra/deferred/deferred_unused_classes_test.dart
tests/compiler/dart2js_extra/mirrors_used_warning2_test.dart
tests/compiler/dart2js_extra/mirrors_used_warning_test.dart
tests/language_2/async_await_foreign_test.dart
tests/language_2/async_call_test.dart
tests/lib_2/async/future_constructor_test.dart
tests/lib_2/async/future_or_bad_type_test.dart
tests/lib_2/async/future_or_strong_test.dart
tests/lib_2/async/intercept_print1_test.dart
tests/lib_2/async/intercept_schedule_microtask1_test.dart
tests/lib_2/async/intercept_schedule_microtask2_test.dart
tests/lib_2/async/run_zoned1_test.dart
tests/lib_2/async/run_zoned4_test.dart
tests/lib_2/async/stream_controller_async_test.dart
tests/lib_2/async/stream_first_where_test.dart
tests/lib_2/async/stream_from_iterable_test.dart
tests/lib_2/async/stream_join_test.dart
tests/lib_2/async/stream_last_where_test.dart
tests/lib_2/async/stream_single_test.dart
tests/lib_2/async/stream_single_to_multi_subscriber_test.dart
tests/lib_2/async/stream_subscription_as_future_test.dart
tests/lib_2/async/stream_transform_test.dart
tests/lib_2/async/stream_transformation_broadcast_test.dart
tests/lib_2/async/stream_transformer_test.dart
tests/lib_2/async/stream_type_test.dart
tests/lib_2/async/timer_not_available_test.dart
tests/lib_2/async/timer_regress22626_test.dart
tests/lib_2/convert/html_escape_test.dart
tests/lib_2/convert/line_splitter_test.dart
tests/lib_2/html/js_interop_constructor_name_div_test.dart
tests/lib_2/html/js_interop_constructor_name_error1_test.dart
tests/lib_2/html/js_interop_constructor_name_error2_test.dart
tests/lib_2/html/js_interop_constructor_name_method_test.dart
tests/lib_2/isolate/spawn_uri_exported_main_test.dart
tests/lib_2/mirrors/invoke_named_test.dart
tests/lib_2/mirrors/invoke_test.dart
tests/lib_2/mirrors/mirrors_used_generic_types_test.dart
tests/standalone_2/http_launch_test.dart
tests/standalone_2/io/code_collection_test.dart
tests/standalone_2/io/file_create_test.dart
tests/standalone_2/io/file_system_delete_test.dart
tests/standalone_2/io/http_10_test.dart
tests/standalone_2/io/http_advanced_test.dart
tests/standalone_2/io/http_auth_digest_test.dart
tests/standalone_2/io/http_auth_test.dart
tests/standalone_2/io/http_close_test.dart
tests/standalone_2/io/http_connection_close_test.dart
tests/standalone_2/io/http_content_length_test.dart
tests/standalone_2/io/http_cookie_date_test.dart
tests/standalone_2/io/http_cookie_test.dart
tests/standalone_2/io/http_cross_process_test.dart
tests/standalone_2/io/http_date_test.dart
tests/standalone_2/io/http_headers_test.dart
tests/standalone_2/io/http_keep_alive_test.dart
tests/standalone_2/io/http_no_reason_phrase_test.dart
tests/standalone_2/io/http_parser_test.dart
tests/standalone_2/io/http_proxy_advanced_test.dart
tests/standalone_2/io/http_proxy_test.dart
tests/standalone_2/io/http_redirect_test.dart
tests/standalone_2/io/http_response_deadline_test.dart
tests/standalone_2/io/http_server_early_client_close2_test.dart
tests/standalone_2/io/http_server_early_client_close_test.dart
tests/standalone_2/io/http_server_response_test.dart
tests/standalone_2/io/http_session_test.dart
tests/standalone_2/io/http_shutdown_test.dart
tests/standalone_2/io/https_server_test.dart
tests/standalone_2/io/https_unauthorized_test.dart
tests/standalone_2/io/issue_22636_test.dart
tests/standalone_2/io/issue_22637_test.dart
tests/standalone_2/io/namespace_test.dart
tests/standalone_2/io/process_start_exception_test.dart
tests/standalone_2/io/process_stderr_test.dart
tests/standalone_2/io/process_stdin_transform_unsubscribe_test.dart
tests/standalone_2/io/process_stdout_test.dart
tests/standalone_2/io/raw_secure_socket_pause_test.dart
tests/standalone_2/io/raw_secure_socket_test.dart
tests/standalone_2/io/raw_socket_cross_process_test.dart
tests/standalone_2/io/raw_socket_write_destroy_test.dart
tests/standalone_2/io/secure_bad_certificate_test.dart
tests/standalone_2/io/secure_socket_renegotiate_test.dart
tests/standalone_2/io/secure_unauthorized_test.dart
tests/standalone_2/io/skipping_dart2js_compilations_test.dart
tests/standalone_2/io/snapshot_fail_test.dart
tests/standalone_2/io/socket_cross_process_test.dart
tests/standalone_2/io/stdout_stderr_non_blocking_test.dart
tests/standalone_2/io/stdout_stderr_test.dart
tests/standalone_2/io/test_extension_test.dart
tests/standalone_2/io/web_socket_ping_test.dart
tests/standalone_2/io/web_socket_pipe_test.dart
tests/standalone_2/io/web_socket_protocol_test.dart
tests/standalone_2/io/web_socket_typed_data_test.dart

Feel free to update with more information if you're better at grepping :).

Also, the following tests has an async main method, which is also most likely an error:

../tests/language_2/async_cascade_test.dart
../tests/language_2/async_finally_rethrow_test.dart
../tests/language_2/await_and_ifnull_test.dart
../tests/language_2/await_in_cascade_test.dart
../tests/language_2/await_null_aware_test.dart
../tests/language_2/await_regression_test.dart
../tests/language_2/custom_await_stack_trace_test.dart
../tests/language_2/deferred_regression_28678_test.dart
../tests/language_2/deferred_super_dependency_test.dart
../tests/language_2/deferred_type_dependency_test.dart
../tests/language_2/generic_async_star_test.dart
../tests/language_2/generic_async_test.dart
../tests/language_2/implicit_downcast_during_return_async_test.dart
../tests/language_2/issue23244_test.dart
../tests/language_2/regress_22438_test.dart
../tests/language_2/regress_22445_test.dart
../tests/language_2/regress_22579_test.dart
../tests/language_2/regress_22728_test.dart
../tests/language_2/regress_23498_test.dart
../tests/language_2/regress_23500_test.dart
../tests/language_2/regress_24935_test.dart
../tests/language_2/regress_26668_test.dart
../tests/language_2/regress_26948_test.dart
../tests/language_2/regress_28278_test.dart
../tests/language_2/shadow_parameter_and_local_test.dart
../tests/language_2/super_in_async1_test.dart
../tests/language_2/super_in_async2_test.dart
../tests/language_2/super_in_async3_test.dart
../tests/language_2/super_in_async4_test.dart
../tests/language_2/super_in_async5_test.dart
../tests/language_2/super_in_async6_test.dart
../tests/language_2/syncstar_yieldstar_test.dart
../tests/language_2/unused_overridden_async_test.dart
../tests/standalone_2/causal_async_stack_test.dart
../tests/standalone_2/deferred_transitive_import_error_test.dart
../tests/standalone_2/packages_file_test.dart
../tests/standalone_2/regress_28854_1_test.dart
../tests/standalone_2/regress_28854_2_test.dart
munificent commented 6 years ago

Thanks for the grepping! I agree expect/minitest + async_helper is the preferred path. For some tests that heavily use asynchrony and group() and test() it may be too much work to do that rewrite today (async_await_test.dart is a good example) which is why unittest is still an allowed short-term choice.

lrhn commented 6 years ago

Something like async_await_test (which also still uses package:unittest, not package:test, even in language_2) is hard thing to rewrite if you don't know exactly what it's testing ... but I've been successful before by just writing test, group and expect functions that do just enough, without trying for full generality.

lrhn commented 6 years ago

As a side note, the following tests still use package:unittest even if it's not Dart 2 compatible:

language_2/async_star_test.dart
language_2/vm/causal_async_exception_stack2_test.dart
language_2/vm/causal_async_exception_stack_test.dart
lib_2/async/stream_controller_async_test.dart
lib_2/async/stream_first_where_test.dart
lib_2/async/stream_from_iterable_test.dart
lib_2/async/stream_iterator_test.dart
lib_2/async/stream_join_test.dart
lib_2/async/stream_last_where_test.dart
lib_2/async/stream_periodic2_test.dart
lib_2/async/stream_periodic3_test.dart
lib_2/async/stream_periodic4_test.dart
lib_2/async/stream_periodic5_test.dart
lib_2/async/stream_periodic6_test.dart
lib_2/async/stream_periodic_test.dart
lib_2/async/stream_single_test.dart
lib_2/async/stream_single_to_multi_subscriber_test.dart
lib_2/async/stream_state_helper.dart
lib_2/async/stream_state_nonzero_timer_test.dart
lib_2/async/stream_state_test.dart
lib_2/async/stream_subscription_as_future_test.dart
lib_2/async/stream_subscription_cancel_test.dart
lib_2/async/stream_timeout_test.dart
lib_2/async/stream_transform_test.dart
lib_2/async/stream_transformation_broadcast_test.dart
lib_2/async/timer_cancel1_test.dart
lib_2/async/timer_cancel2_test.dart
lib_2/async/timer_cancel_test.dart
lib_2/async/timer_isActive_test.dart
lib_2/async/timer_repeat_test.dart
lib_2/async/timer_test.dart
lib_2/html/async_cancellingisolate.dart
lib_2/html/async_oneshot.dart
lib_2/html/async_periodictimer.dart
lib_2/html/async_spawnuri_test.dart
lib_2/html/async_spawnuri_test.dart
lib_2/html/async_test.dart
lib_2/html/async_test.dart
lib_2/html/canvas_test.dart
lib_2/html/canvasrenderingcontext2d_test.dart
lib_2/html/canvasrenderingcontext2d_test.dart
lib_2/html/cross_domain_iframe_test.dart
lib_2/html/css_test.dart
lib_2/html/cssstyledeclaration_test.dart
lib_2/html/custom/attribute_changed_callback_test.dart
lib_2/html/custom/constructor_calls_created_synchronously_test.dart
lib_2/html/custom/created_callback_test.dart
lib_2/html/custom/document_register_basic_test.dart
lib_2/html/custom/document_register_basic_test.dart
lib_2/html/custom/document_register_template_test.dart
lib_2/html/custom/document_register_type_extensions_test.dart
lib_2/html/custom/document_register_type_extensions_test.dart
lib_2/html/custom/element_upgrade_failure_test.dart
lib_2/html/custom/element_upgrade_failure_test.dart
lib_2/html/custom/element_upgrade_test.dart
lib_2/html/custom/element_upgrade_test.dart
lib_2/html/custom/entered_left_view_test.dart
lib_2/html/custom/entered_left_view_test.dart
lib_2/html/custom/js_custom_test.dart
lib_2/html/custom/js_custom_test.dart
lib_2/html/custom/mirrors_2_test.dart
lib_2/html/custom/mirrors_test.dart
lib_2/html/custom/mirrors_test.dart
lib_2/html/custom/regress_194523002_test.dart
lib_2/html/custom/regress_194523002_test.dart
lib_2/html/custom_element_method_clash_test.dart
lib_2/html/custom_element_method_clash_test.dart
lib_2/html/custom_element_name_clash_test.dart
lib_2/html/custom_element_name_clash_test.dart
lib_2/html/custom_elements_23127_test.dart
lib_2/html/custom_elements_23127_test.dart
lib_2/html/custom_elements_test.dart
lib_2/html/custom_elements_test.dart
lib_2/html/element_animate_test.dart
lib_2/html/element_animate_test.dart
lib_2/html/element_test.dart
lib_2/html/element_test.dart
lib_2/html/event_customevent_test.dart
lib_2/html/event_customevent_test.dart
lib_2/html/fileapi_directory_reader_test.dart
lib_2/html/fileapi_directory_reader_test.dart
lib_2/html/fileapi_directory_test.dart
lib_2/html/fileapi_directory_test.dart
lib_2/html/fileapi_entry_test.dart
lib_2/html/fileapi_entry_test.dart
lib_2/html/fileapi_file_entry_test.dart
lib_2/html/fileapi_file_entry_test.dart
lib_2/html/fileapi_file_test.dart
lib_2/html/fileapi_file_test.dart
lib_2/html/fileapi_supported_test.dart
lib_2/html/fileapi_supported_test.dart
lib_2/html/fileapi_supported_throws_test.dart
lib_2/html/fileapi_supported_throws_test.dart
lib_2/html/filereader_test.dart
lib_2/html/filereader_test.dart
lib_2/html/fontface_loaded_test.dart
lib_2/html/fontface_loaded_test.dart
lib_2/html/form_data_test.dart
lib_2/html/form_data_test.dart
lib_2/html/gamepad_test.dart
lib_2/html/gamepad_test.dart
lib_2/html/history_hash_change_test.dart
lib_2/html/history_hash_change_test.dart
lib_2/html/history_history_test.dart
lib_2/html/history_history_test.dart
lib_2/html/history_supports_tests.dart
lib_2/html/history_supports_tests.dart
lib_2/html/history_test.dart
lib_2/html/history_test.dart
lib_2/html/indexeddb_1_test.dart
lib_2/html/indexeddb_1_test.dart
lib_2/html/indexeddb_2_test.dart
lib_2/html/indexeddb_2_test.dart
lib_2/html/indexeddb_3_test.dart
lib_2/html/indexeddb_3_test.dart
lib_2/html/indexeddb_4_test.dart
lib_2/html/indexeddb_4_test.dart
lib_2/html/indexeddb_5_test.dart
lib_2/html/indexeddb_5_test.dart
lib_2/html/interactive_geolocation_test.dart
lib_2/html/interactive_geolocation_test.dart
lib_2/html/interactive_media_test.dart
lib_2/html/interactive_media_test.dart
lib_2/html/interactive_test.dart
lib_2/html/interactive_test.dart
lib_2/html/isolates_test.dart
lib_2/html/isolates_test.dart
lib_2/html/js_array_test.dart
lib_2/html/js_array_test.dart
lib_2/html/js_dart_to_string_test.dart
lib_2/html/js_dart_to_string_test.dart
lib_2/html/js_dispatch_property_test.dart
lib_2/html/js_dispatch_property_test.dart
lib_2/html/js_function_getter_trust_types_test.dart
lib_2/html/js_function_getter_trust_types_test.dart
lib_2/html/js_function_getter_trust_types_test.dart
lib_2/html/js_interop_1_test.dart
lib_2/html/js_interop_1_test.dart
lib_2/html/js_interop_constructor_name_div_test.dart
lib_2/html/js_interop_constructor_name_div_test.dart
lib_2/html/js_interop_constructor_name_error1_test.dart
lib_2/html/js_interop_constructor_name_error1_test.dart
lib_2/html/js_interop_constructor_name_error2_test.dart
lib_2/html/js_interop_constructor_name_error2_test.dart
lib_2/html/js_interop_constructor_name_method_test.dart
lib_2/html/js_interop_constructor_name_method_test.dart
lib_2/html/worker_api_test.dart
lib_2/html/worker_api_test.dart
lib_2/html/worker_test.dart
lib_2/html/worker_test.dart
lib_2/html/xhr_cross_origin_test.dart
lib_2/html/xhr_cross_origin_test.dart
lib_2/html/xhr_test.dart
lib_2/html/xhr_test.dart
lib_2/isolate/browser/compute_this_script_browser_test.dart
lib_2/isolate/browser/compute_this_script_browser_test.dart
lib_2/isolate/browser/package_resolve_browser_hook2_test.dart
lib_2/isolate/browser/package_resolve_browser_hook2_test.dart
lib_2/isolate/browser/package_resolve_browser_hook_test.dart
lib_2/isolate/browser/package_resolve_browser_hook_test.dart
lib_2/isolate/browser/package_resolve_browser_test.dart
lib_2/isolate/browser/package_resolve_browser_test.dart
lib_2/isolate/browser/typed_data_message_test.dart
lib_2/isolate/count_test.dart
lib_2/isolate/cross_isolate_message_test.dart
lib_2/isolate/deferred_in_isolate2_test.dart
lib_2/isolate/mandel_isolate_test.dart
lib_2/isolate/message2_test.dart
lib_2/isolate/message_test.dart
lib_2/isolate/mint_maker_test.dart
lib_2/isolate/nested_spawn2_test.dart
lib_2/isolate/nested_spawn_test.dart
lib_2/isolate/raw_port_test.dart
lib_2/isolate/remote_unittest_helper.dart
lib_2/isolate/request_reply_test.dart
lib_2/isolate/spawn_function_custom_class_test.dart
lib_2/isolate/spawn_function_test.dart
lib_2/isolate/spawn_uri_multi_test.dart
lib_2/isolate/spawn_uri_nested_vm_test.dart
lib_2/isolate/spawn_uri_test.dart
lib_2/isolate/spawn_uri_vm_test.dart
lib_2/isolate/static_function_test.dart
lib_2/isolate/timer_isolate_test.dart
lib_2/isolate/unresolved_ports_test.dart
lib_2/mirrors/library_uri_io_test.dart
lib_2/mirrors/library_uri_package_test.dart

That should probably also be fixed.

munificent commented 6 years ago

As a side note, the following tests still use package:unittest even if it's not Dart 2 compatible

unittest is strong mode clean (which I assume more or less implies "Dart 2 compatible"), so it's not a burning issue to move those tests off of it. It's more that unittest is deprecated and no longer maintained.