libservo: Stop using the custom test harness and run_api_tests#40131
libservo: Stop using the custom test harness and run_api_tests#40131mrobinson merged 1 commit intoservo:mainfrom
run_api_tests#40131Conversation
servo-highfive
added
the
S-awaiting-review
|
cc @jschwe |
jschwe
left a comment
jschwe
left a comment
There was a problem hiding this comment.
Ug, that unfortunate. I wonder what went wrong there. Sorry about that.
But I guess removing the custom harness where not necessary is anyway a good step.
I'll take a closer look after lunch
| // my-test-2: test | ||
| // ``` | ||
| if args.contains(&String::from("--list")) { | ||
| println!("test_multiprocess_preference_observer: test") |
There was a problem hiding this comment.
I think we would want to exit here (and not run the test), otherwise an error in the test, would cause the test list gathering step to also fail.
| // my-test-2: test | ||
| // ``` | ||
| if args.contains(&String::from("--list")) { | ||
| println!("test_multiprocess_preference_observer: test") |
There was a problem hiding this comment.
Should we return here after printing the list of tests, to avoid increasing the runtime of the harness when only listing?
There was a problem hiding this comment.
Oh. Curious that this worked at all. This is fixed now.
| println!("test_multiprocess_preference_observer: test") | ||
| } | ||
|
|
||
| test_multiprocess_preference_observer(); |
There was a problem hiding this comment.
Should this be guarded by a check on --nocapture --exact being present in args?
There was a problem hiding this comment.
I think it is better to be flexible in the arguments allowed and the order that they are delivered in. I believe that expecting them in a certain order was causing the issue in the first place.
mrobinson
force-pushed
the
fix-api-tests
branch
from
28ecfb5 to
055bfca
Compare
The changes in servo#39812 seem to have completely disabled the API tests, so this PR gets them running again by removing the custom test harness (apart from the multiprocess test). The custom test harness is typically not necessary now that all tests are run in their own process. In addition, the behavior of the `WebView::page_zoom()` is fixed when set via the `viewport` meta tag. This functionality was broken, but the unit test wasn't catching it. There is still one failing test, but that is tracked via servo#40129. Signed-off-by: Martin Robinson <[email protected]>
mrobinson
force-pushed
the
fix-api-tests
branch
from
055bfca to
a87b959
Compare
servo-highfive
removed
the
S-awaiting-review
No problem. The switch to nexttest does allow us to remove the custom harness in the general case, which is very nice. |
servo-highfive
added
the
S-awaiting-merge
servo-highfive
removed
the
S-awaiting-merge
4 participants
The changes in #39812 seem to have completely disabled the API tests, so
this PR gets them running again by removing the custom test harness
(apart from the multiprocess test). The custom test harness is typically
not necessary now that all tests are run in their own process.
In addition, the behavior of the
WebView::page_zoom()is fixed whenset via the
viewportmeta tag. This functionality was broken, but theunit test wasn't catching it.
There is still one failing test, but that is tracked via #40129.
Testing: This change fixes the tests.