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
Preserve logs from retries as they have data! #19341
Conversation
62e1c3f
to
4085d87
Compare
public interface IScreenshotSupportedApp : IApp | ||
{ | ||
FileInfo Screenshot(string fileName); | ||
byte[] Screenshot(); | ||
string ElementTree { get; } | ||
} |
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.
"optional APIs" should not be forced on the core testing IApp
. Things like device logs and screenshots are only available on some platforms and in somace situations.
string filename = $"{fileName}.png"; | ||
Screenshot screenshot = _driver.GetScreenshot(); |
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.
Don't force the .png
here (even though it is a png) since this makes the other part of the code hard to reuse.
string? GetGeneratedFilePath(string filename, string? note = null) | ||
{ |
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.
This new method makes sure to get a new filename per run so that all logs are preserved.
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.
But can we have a way to pass the guid from the cake? so we can map to what is on ci ui display name?
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.
Not sure, there might be multiple guids per run. If the test run crashes or appium somehow needs a restart, then we may have 2 items per single cake run.
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, which the exception of the terminology thing I mentioned
4085d87
to
00709d3
Compare
Description of Change
Some logs have valuable data, so make sure to give them a unique name.
Part of #19038