autofill: Remove redundant RunUntilIdle() calls in FormAutofillTest [chromium/src : main]
Tamino Bauknecht (Gerrit)
Tamino Bauknecht added 1 comment![]( "Open in Gerrit")
@jihad...@google.com I removed my previous changes in `RenderViewTest` because I noticed that `ExecuteJavaScriptForTests` does not even need the `RunLoop` and it seems like the form extraction is triggered manually which also shouldn't need a `RunLoop`.
I ran the test case for 200 times and didn't experience any flakiness on my local machine, do you think this change could cause issues? Initially, I coupled the end of the `RunLoop` to the end of the javascript execution, but that doesn't actually make sense because the execution was already synchronous before. Do you see an issue removing the `RunLoop` and if yes, can you maybe explain what I can use to call `base::RunLoop::Quit()`?
Related details
- Jihad Hanna
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jihad Hanna (Gerrit)
Jihad Hanna voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jihad Hanna
| Code-Review | +1 |
@jihad...@google.com I removed my previous changes in `RenderViewTest` because I noticed that `ExecuteJavaScriptForTests` does not even need the `RunLoop` and it seems like the form extraction is triggered manually which also shouldn't need a `RunLoop`.
I ran the test case for 200 times and didn't experience any flakiness on my local machine, do you think this change could cause issues? Initially, I coupled the end of the `RunLoop` to the end of the javascript execution, but that doesn't actually make sense because the execution was already synchronous before. Do you see an issue removing the `RunLoop` and if yes, can you maybe explain what I can use to call `base::RunLoop::Quit()`?
That's very interesting. I had never checked whether `ExecuteJavaScriptForTests()` was synchronous or not, I always assumed it was async.
If it is synchronous I agree there's no point in having them, but I guess there's so many instances around our test code where these waiting mechanisms around `ExecuteJavaScriptForTests()`
Hey Jan, Could you please take a second look? Thank you!
Related details
- Jan Keitel
- Tamino Bauknecht
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jan Keitel
| Code-Review | +1 |
Jihad Hanna@jihad...@google.com I removed my previous changes in `RenderViewTest` because I noticed that `ExecuteJavaScriptForTests` does not even need the `RunLoop` and it seems like the form extraction is triggered manually which also shouldn't need a `RunLoop`.
I ran the test case for 200 times and didn't experience any flakiness on my local machine, do you think this change could cause issues? Initially, I coupled the end of the `RunLoop` to the end of the javascript execution, but that doesn't actually make sense because the execution was already synchronous before. Do you see an issue removing the `RunLoop` and if yes, can you maybe explain what I can use to call `base::RunLoop::Quit()`?
That's very interesting. I had never checked whether `ExecuteJavaScriptForTests()` was synchronous or not, I always assumed it was async.
If it is synchronous I agree there's no point in having them, but I guess there's so many instances around our test code where these waiting mechanisms around `ExecuteJavaScriptForTests()`
This looks fine to me. We do asynchronous work in `FormTracker`, but since we already trigger extraction manually, that does not seem relevant.
@tam...@chromium.org, on your more general question:
Normally, the recommended pattern is something along the following lines:
```
base::RunLoop run_loop;
my_object.DoSomethingAsync(/*on_completion_callback=*/run_loop.QuitClosure());
run_loop.Run();
// continue testing other things - run only once the quit closure is called
```
`Run()` then starts a new run loop that is only terminated when the completion callback is run.
Related details
- Tamino Bauknecht
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tamino Bauknecht (Gerrit)
Tamino Bauknecht added 1 comment![]( "Open in Gerrit")
Jihad Hanna@jihad...@google.com I removed my previous changes in `RenderViewTest` because I noticed that `ExecuteJavaScriptForTests` does not even need the `RunLoop` and it seems like the form extraction is triggered manually which also shouldn't need a `RunLoop`.
I ran the test case for 200 times and didn't experience any flakiness on my local machine, do you think this change could cause issues? Initially, I coupled the end of the `RunLoop` to the end of the javascript execution, but that doesn't actually make sense because the execution was already synchronous before. Do you see an issue removing the `RunLoop` and if yes, can you maybe explain what I can use to call `base::RunLoop::Quit()`?
Jan KeitelThat's very interesting. I had never checked whether `ExecuteJavaScriptForTests()` was synchronous or not, I always assumed it was async.
If it is synchronous I agree there's no point in having them, but I guess there's so many instances around our test code where these waiting mechanisms around `ExecuteJavaScriptForTests()`
This looks fine to me. We do asynchronous work in `FormTracker`, but since we already trigger extraction manually, that does not seem relevant.
@tam...@chromium.org, on your more general question:
Normally, the recommended pattern is something along the following lines:
```
base::RunLoop run_loop;
my_object.DoSomethingAsync(/*on_completion_callback=*/run_loop.QuitClosure());
run_loop.Run();// continue testing other things - run only once the quit closure is called
````Run()` then starts a new run loop that is only terminated when the completion callback is run.
Acknowledged, @jke...@google.com thank you for your explanation! I've actually already used that pattern in an earlier patchset :) It is just hard to find the correct function on the correct observer for an `EXPECT_CALL` and so far none of the methods I needed it for directly accepted such a completion callback out-of-box :|
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jan Keitel (Gerrit)
Jan Keitel added 1 comment![]( "Open in Gerrit")
Jihad Hanna@jihad...@google.com I removed my previous changes in `RenderViewTest` because I noticed that `ExecuteJavaScriptForTests` does not even need the `RunLoop` and it seems like the form extraction is triggered manually which also shouldn't need a `RunLoop`.
I ran the test case for 200 times and didn't experience any flakiness on my local machine, do you think this change could cause issues? Initially, I coupled the end of the `RunLoop` to the end of the javascript execution, but that doesn't actually make sense because the execution was already synchronous before. Do you see an issue removing the `RunLoop` and if yes, can you maybe explain what I can use to call `base::RunLoop::Quit()`?
Jan KeitelThat's very interesting. I had never checked whether `ExecuteJavaScriptForTests()` was synchronous or not, I always assumed it was async.
If it is synchronous I agree there's no point in having them, but I guess there's so many instances around our test code where these waiting mechanisms around `ExecuteJavaScriptForTests()`
Tamino BauknechtThis looks fine to me. We do asynchronous work in `FormTracker`, but since we already trigger extraction manually, that does not seem relevant.
@tam...@chromium.org, on your more general question:
Normally, the recommended pattern is something along the following lines:
```
base::RunLoop run_loop;
my_object.DoSomethingAsync(/*on_completion_callback=*/run_loop.QuitClosure());
run_loop.Run();// continue testing other things - run only once the quit closure is called
````Run()` then starts a new run loop that is only terminated when the completion callback is run.
Acknowledged, @jke...@google.com thank you for your explanation! I've actually already used that pattern in an earlier patchset :) It is just hard to find the correct function on the correct observer for an `EXPECT_CALL` and so far none of the methods I needed it for directly accepted such a completion callback out-of-box :|
Yep, that's unfortunately often true - which is why `RunUntilIdle` still gets used frequently.
Another variant is [`base::test::RunUntil`](https://source.chromium.org/chromium/chromium/src/+/main:base/test/run_until.h;l=34;drc=018839f06e031df1485167f95948ed2b81f87a1f), but AFAIK that's vulnerable to the same issue as `RunUntilIdle` - if, somehow, a machine is extremely busy, it may time out, because it also waits for a moment in which the task loop becomes idle before attempting to verify the condition again.
Related details
- Tamino Bauknecht
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tamino Bauknecht (Gerrit)
Tamino Bauknecht voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
7 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
autofill: Remove redundant RunUntilIdle() calls in FormAutofillTest
Since `RenderViewTest::ExecuteJavaScriptForTests` is already synchronous
and the asynchronous form extraction is also manually triggered
synchronously afterwards by `UpdateFormCache`, no `RunLoop` is required.
There should be no cross-process communication either, so the calls to
`base::RunLoop::RunUntilIdle()` were removed.
- M chrome/renderer/autofill/form_autofill_browsertest.cc
Code-Review: +1 by Jan Keitel, +1 by Jihad Hanna
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |