Skip to content
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

Added numbering to the filenames of the attachments that are extracted #38

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bennet3
Copy link

@bennet3 bennet3 commented Dec 4, 2019

Change Description:
Added numbering to the filenames of the attachments that are extracted.
I needed the attachments to be numbered so that I can see the order that the screenshots happened for UI testing and debugging.
Test Plan/Testing Performed:
Verified that the numbering of the attachments works by running on several .xcresult files from my own project

if let identifier = attachment.payloadRef?.id {
self.id = identifier;

// Now let's figure out the filename & path
let filename = attachment.filename ?? identifier
let filename = filenamePrefix + "\(attachment.filename ?? identifier)"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the interpolated string actually necessary? I assume the following would also work:

filenamePrefix + (attachment.filename ?? identifier)

@rpendleton
Copy link

rpendleton commented Dec 4, 2019

This will result in different filenames, so depending on the release cycle of the project, it may be considered a breaking change. It might be good to disable the indexes by default and enable them with a command line argument, or save this change until a major semver release.

Copy link
Collaborator

@abotkin-cpi abotkin-cpi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @bennet3! As @rpendleton mentioned, there are folks who'll want the original filenames, so this functionality would need to use a flag similar to what we do for dividing screenshots (like "--model" which you can follow from ScreenshotsCommand.swift:43 to see how it works).

If we make this new flag ("--ordered" perhaps), what would the expected behavior be for this flag? Is it so that all the screenshots under an individual test will be prefixed with an index that matches the order of the screenshot in test progression & should apply regardless of whether it's an automatic or user-generated screenshot? So if there is a test A with one screenshot & test B with three screenshots, test A's screenshot would get 1_<originalName> & test B's screenshot would have 1_<originalName>, 2_<originalName>, 3_<originalName> to show that screenshots happened in that order? Asking that as we'll want to update the README.md to have information about how it works & the xcparse screenshots --help command would need to give a helpful hint on what this option would do.

@bennet3 If this is something you need to use in your CI immediately, you can hook up your fork for Homebrew installation. If you'd like me to write some instructions on how to do so & maybe add those instructions into either this or the Homebrew tap's README.md, let me know.

Future Considerations

I'd be curious to get folks input on whether we may want to have a future feature where you can specify how the screenshot names are formulated, so that you could do things like stipulate screenshots should have the naming <model><os>_<test>_<ordering_number>_<originalScreenshotName>.png. We have the directory sub-division, but I could see some folks (coughBitrise) wanting the screenshots in a flat directory structure but well-named & the names being customizable.

@abotkin-cpi abotkin-cpi self-assigned this Jan 13, 2020
@abotkin-cpi
Copy link
Collaborator

Assigning to myself. We're looking at adding a feature in order to created animated PNGs from the test failure screenshots & to do that, we'll need to create the ordered screenshots feature. Additionally, to support both that & this PR, xcparse has to be re-worked to do a DFS rather than BFS of the tree for attachments/screenshots. While this current PR will look like it's in order most of the time, it can be out of order due to the BFS for attachments rather than DFS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants