-
Notifications
You must be signed in to change notification settings - Fork 6k
[web] improving felt: running target unit tests, cleaning up used proccesses #17339
Conversation
| temporaryDirectories.add(_drivers); | ||
|
|
||
| io.Directory temp = io.Directory.current; | ||
| io.Directory.current = _browserDriverDir; |
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.
Is this done only to resolve ./chromedriver/chromedriver from the current directory? If so, I'd recommend calling startProcess(pathlib.join(_browserDriverDir.path, 'chromedriver', 'chromedriver')) with the same effect but without changing global program state. Same for workingDirectory.
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.
+1
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.
Unfortunately we also need it since the library we are importing from web_drivers also need it.
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.
in other words this line is necessary for both line 98 and 99. Suggestion solution only considers line 99.
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.
I see.
mdebbar
left a comment
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.
LGTM after addressing Yegor's comments.
| temporaryDirectories.add(_drivers); | ||
|
|
||
| io.Directory temp = io.Directory.current; | ||
| io.Directory.current = _browserDriverDir; |
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.
+1
|
Merging the PR, since it is only related to web engine testing tools not related to breakage or to the engine code itself. |
Improvements on felt:
Fixes: flutter/flutter#53320