[M] Change in dart/sdk[main]: DAS session logger: Normalize paths as log entries are written.
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
Hi Brian, I manually tested this a few times; seems to work just fine. I'm not sure how to rigorously test it though... I guess I can do more manual testing, with moving the Dart SDK path and the project paths, etc. But I don't see any unit testing. Should I just create some fresh unit tests, or are there session logger tests I missed?
Thanks much!
Related details
- Brian Wilkerson
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Brian Wilkerson (Gerrit)
Brian Wilkerson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Brian Wilkerson
| Code-Review | +1 |
3 comments
Hi Brian, I manually tested this a few times; seems to work just fine. I'm not sure how to rigorously test it though... I guess I can do more manual testing, with moving the Dart SDK path and the project paths, etc. But I don't see any unit testing. Should I just create some fresh unit tests, or are there session logger tests I missed?
Thanks much!
As far as I know we don't have any tests. The code started under `tools` and we don't normally write tests for tools. It wasn't until we decided to connect it to the insight pages that it moved under `src`.
By the way, I'm hoping to eventually replace the instrumentation log with a session log. I don't know whether having the log normalized by default is good for that or better. Just something to think about.
_sessionLogger.addPackageRoots(I hadn't really thought about this before, but this is interesting. I think this means that the logger can keep up with changes to the `package_config.json` files. We were almost certainly previously depending on the package resolution to be constant across the session, which was always limiting.
_sessionLogger = SessionLogger(filePath: sessionLogFilePath);Does this break the ability for the user to delay recording a session until later (after they click the button on the insight pages)?
Related details
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 2 comments![]( "Open in Gerrit")
_sessionLogger.addPackageRoots(I hadn't really thought about this before, but this is interesting. I think this means that the logger can keep up with changes to the `package_config.json` files. We were almost certainly previously depending on the package resolution to be constant across the session, which was always limiting.
Ah good thinking. I guess there will be cases where the package config changes, like a performance issue caused by `git checkout` where the package config changes, etc.
_sessionLogger = SessionLogger(filePath: sessionLogFilePath);Does this break the ability for the user to delay recording a session until later (after they click the button on the insight pages)?
No, I tested this manually with the insights page button, and it still works.
The logic is the same as before: `sessionLogFilePath` is either null, or not null, and the new SessionLogger constructor passes the value to the SessionLoggerInMemorySink constructor, which instantiates its `_nextLogger` depending on whether the value is null or not, effectively the same as before.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Konstantin Shcheglov (Gerrit)
Konstantin Shcheglov voted Code-Review+1![]( "Open in Gerrit")
- Samuel Rawlins
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins added 1 comment![]( "Open in Gerrit")
Brian WilkersonHi Brian, I manually tested this a few times; seems to work just fine. I'm not sure how to rigorously test it though... I guess I can do more manual testing, with moving the Dart SDK path and the project paths, etc. But I don't see any unit testing. Should I just create some fresh unit tests, or are there session logger tests I missed?
Thanks much!
As far as I know we don't have any tests. The code started under `tools` and we don't normally write tests for tools. It wasn't until we decided to connect it to the insight pages that it moved under `src`.
By the way, I'm hoping to eventually replace the instrumentation log with a session log. I don't know whether having the log normalized by default is good for that or better. Just something to think about.
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Samuel Rawlins (Gerrit)
Samuel Rawlins voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |