search

web-platform-tests / rfcs

web-platform-tests RFCs
75 stars 63 forks source link

RFC 58: Add a new test property to control displaying test state updates #58

Closed sefeng211 closed 4 years ago

sefeng211 commented 4 years ago

test changes: https://github.com/web-platform-tests/wpt/pull/24170

jgraham commented 4 years ago

It's kind of unclear if blocking notify_test_state is actually the right approach, since in theory other things could be using that (e.g. if we changed the harness to stream events back and used that to build an interactive view of the test state). It might make more sense to just make the Output handler not register a callback in case this property is set.

sefeng211 commented 4 years ago

@stephenmcgruer @jgraham Yeah....good point!

How about we just not register the show_status() function to test_state_callbacks and test_done_callbacks?

So this test property could be called register_show_status_callback?

jgraham commented 4 years ago

I think we should name the property for the desired effect rather than the implementation strategy. To me notify_test_state seemed OK because we would stop doing in-content notifications, but I see that it's confusing if you think of it as blocking all kinds of notifications. So we could maybe have output_test_state: false or allow_harness_dom_updates: false or something.

sefeng211 commented 4 years ago

@jgraham sounds good, what do you think about the implementation strategy, Stephen mentioned a good point that we don't want to output anything between tests.

jgraham commented 4 years ago

I don't show_results in the output function is only called at most once when the tests are complete. We don't do interactive update of the table of results. So I think as long as show_status isn't called it's probably fine.

sefeng211 commented 4 years ago

I updated the WPT patch to change the implementation from checking the flag in notify_test_state to not calling the show_status function in the callback. Detail: https://github.com/web-platform-tests/wpt/pull/24170/commits/255548294450d02c4465e62c8a434dc12faf55a5, @stephenmcgruer Please let me know what you think :)

sefeng211 commented 4 years ago

I left the flag name as hide_test_state because we want the default value to be false.

sefeng211 commented 4 years ago

Updated!

sefeng211 commented 4 years ago

@Ms2ger Does it look good to you? (pinging in case it falls off your radar)

stephenmcgruer commented 4 years ago

Checked with jgraham that they're happy too, so that's the 1-week LGTM-no-objections time passed. Merging.