[omnibox][webui] BrowserActionInteractiveTest & ...ViewsTest [chromium/src : main]
manuk hovanesian (Gerrit)
manuk hovanesian voted and added 1 comment![]( "Open in Gerrit")
Votes added by manuk hovanesian
| Auto-Submit | +1 |
1 comment
Alex, please review browser_test_utils.cc|h
Solomon, please review browser_action_interactive_test.cc
Related details
- Alex Moshchuk
- Solomon Kinard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Solomon Kinard (Gerrit)
Solomon Kinard added 1 comment![]( "Open in Gerrit")
- Alex Moshchuk
- manuk hovanesian
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
manuk hovanesian (Gerrit)
manuk hovanesian voted and added 1 comment![]( "Open in Gerrit")
Votes added by manuk hovanesian
| Auto-Submit | +1 |
1 comment
Review before tests pass?
tests have passed now.
will wait for tests to pass next time
Related details
- Alex Moshchuk
- Solomon Kinard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
manuk hovanesian (Gerrit)
manuk hovanesian added 1 comment![]( "Open in Gerrit")
Moving Alex from reviewer to +cc because ooo.
Dave, please review browser_test_utils.cc|h
Related details
- Alex Moshchuk
- Dave Tapuska
- Solomon Kinard
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 4 comments![]( "Open in Gerrit")
BrowserActionInteractiveTest() {Can you explain why these tests aren't parameterized? It would seem that webui omnibox is not enabled by default so we should likely be testing both systems no?
I would have expected that you are actually running these tests part of another bot with the feature actually enabled. (see how the bfcache tests were done)
Then you could adjust the test framework for it to work in either mode.
content::CreateAndLoadWebContentsObserver frame_observer(3);Perhaps the CreateAndLoadWebContentsObserver should ignore webUI by default? And only put it into a mode where it listens to webUI?
"Omnibox Popup", "Omnibox Popup",I think perhaps GetWebContentTitles should ignore webUI. Having "Omnibox Popup" spread throughout all the tests is not desirable, and we should just filter it in one spot. (or use IsSubsetOf as I indicated below).
"DevTools", "Omnibox Popup", "Omnibox Popup"));Same here... Plumbing the names in is not helpful. This should just be a testing::IsSubsetOf(...)
Related details
- Alex Moshchuk
- Solomon Kinard
- manuk hovanesian
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. |
Solomon Kinard (Gerrit)
Solomon Kinard added 1 comment![]( "Open in Gerrit")
Add me back to the attention set after comments have been addressed?
Related details
- Alex Moshchuk
- manuk hovanesian
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. |
manuk hovanesian (Gerrit)
manuk hovanesian voted and added 4 comments![]( "Open in Gerrit")
Votes added by manuk hovanesian
| Auto-Submit | +1 |
4 comments
BrowserActionInteractiveTest() {Can you explain why these tests aren't parameterized? It would seem that webui omnibox is not enabled by default so we should likely be testing both systems no?
I would have expected that you are actually running these tests part of another bot with the feature actually enabled. (see how the bfcache tests were done)
Then you could adjust the test framework for it to work in either mode.
these features are in the process of launching in m145 & m146.
We don't usually test both enabled and disabled flows in every single test when adding new features. usually, just the core functionality related to feature gets tests with both enabled/disabled paths. but unrelated tests like these, it's less effort to just test the most-likely-to-launch flow and update the tests if we decide otherwise.
for these features specifically, we have about 1000 failing tests when these 2 features are enabled. we'd have to add a bunch of `if(param enabled) /*new test expectations*/ else /*old test expectations*/` to these tests,
Then we'd have go back through those tests and clean up the old branch in a month or 2 when the features launch.
content::CreateAndLoadWebContentsObserver frame_observer(3);Perhaps the CreateAndLoadWebContentsObserver should ignore webUI by default? And only put it into a mode where it listens to webUI?
done.
can't just ignore all web-uis' webcontents, because some tests are interested in a particular webui webcontent. e.g. dev tools for 1 test in this file, but also some other webUIs in other tests.
so i instead made the observer take an optional callback to filter which webui's are ignored.
I think perhaps GetWebContentTitles should ignore webUI. Having "Omnibox Popup" spread throughout all the tests is not desirable, and we should just filter it in one spot. (or use IsSubsetOf as I indicated below).
done, observer now ignores omnibox webui, so reverted all of these expect_that title checks
Same here... Plumbing the names in is not helpful. This should just be a testing::IsSubsetOf(...)
done, observer now ignores omnibox webui, so reverted all of these expect_that title checks
Related details
- Dave Tapuska
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. |
Dave Tapuska (Gerrit)
Dave Tapuska added 1 comment![]( "Open in Gerrit")
BrowserActionInteractiveTest() {manuk hovanesianCan you explain why these tests aren't parameterized? It would seem that webui omnibox is not enabled by default so we should likely be testing both systems no?
I would have expected that you are actually running these tests part of another bot with the feature actually enabled. (see how the bfcache tests were done)
Then you could adjust the test framework for it to work in either mode.
these features are in the process of launching in m145 & m146.
We don't usually test both enabled and disabled flows in every single test when adding new features. usually, just the core functionality related to feature gets tests with both enabled/disabled paths. but unrelated tests like these, it's less effort to just test the most-likely-to-launch flow and update the tests if we decide otherwise.
for these features specifically, we have about 1000 failing tests when these 2 features are enabled. we'd have to add a bunch of `if(param enabled) /*new test expectations*/ else /*old test expectations*/` to these tests,
Then we'd have go back through those tests and clean up the old branch in a month or 2 when the features launch.
- manuk hovanesian
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. |
manuk hovanesian (Gerrit)
manuk hovanesian voted and added 1 comment![]( "Open in Gerrit")
Votes added by manuk hovanesian
| Auto-Submit | +1 |
1 comment
BrowserActionInteractiveTest() {manuk hovanesianCan you explain why these tests aren't parameterized? It would seem that webui omnibox is not enabled by default so we should likely be testing both systems no?
I would have expected that you are actually running these tests part of another bot with the feature actually enabled. (see how the bfcache tests were done)
Then you could adjust the test framework for it to work in either mode.
Dave Tapuskathese features are in the process of launching in m145 & m146.
We don't usually test both enabled and disabled flows in every single test when adding new features. usually, just the core functionality related to feature gets tests with both enabled/disabled paths. but unrelated tests like these, it's less effort to just test the most-likely-to-launch flow and update the tests if we decide otherwise.
for these features specifically, we have about 1000 failing tests when these 2 features are enabled. we'd have to add a bunch of `if(param enabled) /*new test expectations*/ else /*old test expectations*/` to these tests,
Then we'd have go back through those tests and clean up the old branch in a month or 2 when the features launch.
But with the filter won't these tests now pass either way?
ah, right, just like the other CL, now that we're filtering out, it passes regardless.
done
Related details
- Dave Tapuska
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dave Tapuska (Gerrit)
Dave Tapuska added 3 comments![]( "Open in Gerrit")
class WebContentsObserver : public content::CreateAndLoadWebContentsObserver {Can we name this something other than WebContentsObserver, something perhaps more descriptive. CreateAndLoadUserWebContentsObserver maybe?
std::vector<raw_ptr<WebContents, DanglingUntriaged>> web_contents_;You aren't listening for WebContents destoryed, and these dangling ptrs are dangerous. You can grab the web_contents from each LoadStopObserver anyways.
CreateAndLoadWebContentsObserver::~CreateAndLoadWebContentsObserver() {}Please fix this WARNING reported by ClangTidy: check: modernize-use-equals-default
use '= default' to define a trivial destruc...
check: modernize-use-equals-default
use '= default' to define a trivial destructor (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-equals-default.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-equals-default` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel` or `mac-clang-tidy-rel`)
Related details
- Solomon Kinard
- manuk hovanesian
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. |