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

Use attachment timestamp as modification date of exported attachment file #82

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

Conversation

Boerworz
Copy link

@Boerworz Boerworz commented Apr 12, 2023

Change Description
Hi!

First of all, thank you for building this library! It's been very useful for us in making screenshots available on Bitrise for our failing UI tests.

However, as you are aware, a lot of people (including us) would find it convenient to be able to view the exported screenshots in the order they were taken in the tests. I saw that there have been some discussions about this previously (#38, #42), but as far as I could tell nothing concrete has come off it, so I decided to take a stab at it!

Instead of solving the problem by changing the exported filename—which requires a lot of considerations as seen in #38—I opted to simply set the modification date of the exported file. Prior to this change the modification date would be the date of when xcparse wrote the exported file to disk. Setting the modification date instead to the attachment's timestamp means that the screenshots can be viewed in the order they were taken simply by sorting by the modification date. This is supported out-of-the box in Finder and using the -t flag for ls in the terminal.

Test Plan/Testing Performed
It feels like quite a safe change, so I've only performed a little bit of manual testing on an xcresult bundle. However I'd be happy to get some help with writing unit tests and/or doing some more manual testing on other result bundles!

…file

This allows users of `xcparse screenshots` to order the exported
screenshots by the screenshots' timestamps in Finder, for example. This
makes it much easier to understand how the UI tests progressed.
@Boerworz
Copy link
Author

Welp, turns out that even though APFS has nanosecond precision for the file modification date, the zip format only has 1 second precision for each file IIUC. This means that my changes in this PR work great when testing locally, but does not work as great when the screenshots are zipped to make them available as a build artefact on Bitrise. If there are multiple screenshots taken during the same second then these might be incorrectly ordered after unzipping them.

@abotkin-cpi @mgorkani: Let me know what you want to do with my changes! I still think setting the modification date is a good default behaviour to have even if it didn't quite solve the problem for my personal setup.

@abotkin-cpi
Copy link
Collaborator

@rsukumar-cpi Could you help review within the next week?

@Boerworz Boerworz requested a review from abotkin-cpi April 19, 2023 10:58
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.

Changes seem fair and unit tests are passing with our example xcresults

@rsukumar-cpi could you take a review as well? Thinking we can line this up for a new minor version bump next week

Copy link
Contributor

@rsukumar-cpi rsukumar-cpi left a comment

Choose a reason for hiding this comment

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

The changes seem safe and reasonable to me.

@abotkin-cpi
Copy link
Collaborator

Plan is to merge this early next week and cut a new build of xcparse by Wednesday.

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.

3 participants