-
Notifications
You must be signed in to change notification settings - Fork 29.7k
Remove run in shell and add unit test for chrome launching #39462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove run in shell and add unit test for chrome launching #39462
Conversation
Codecov Report
@@ Coverage Diff @@
## master #39462 +/- ##
==========================================
+ Coverage 56.79% 57.26% +0.47%
==========================================
Files 195 195
Lines 18506 18506
==========================================
+ Hits 10510 10597 +87
+ Misses 7996 7909 -87
Continue to review full report at Codecov.
|
| Future<Chrome> launch(String url, { bool headless = false, bool skipCheck = false }) async { | ||
| final String chromeExecutable = findChromeExecutable(); | ||
| final Directory dataDir = fs.systemTempDirectory.createTempSync(); | ||
| final Directory dataDir = fs.directory('.dart_tool'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be something more specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it fills up the folder with junk, will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
Ignoring add2app-macos which was disabled for repeated failures. Will land once build goes green |
| await chrome.chromeConnection.getTabs(); | ||
| } catch (e) { | ||
| await chrome.close(); | ||
| print('here'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray debug print
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #39751
| })); | ||
| } | ||
|
|
||
| class MockProcessManager extends Mock implements ProcessManager {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MockProcessManager defined in mocks.dart looks like it does what you want.
Description
Fixes chrome launching on windows by removing runInShell. Places the user data directory under .dart_tool and removes code that deletes it on tear down.
Fixes #39213
Fixes #38537