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

Update settings migration workflow #7012

Closed

Conversation

mojganii
Copy link
Collaborator

@mojganii mojganii commented Oct 18, 2024

This PR optimizes the testing workflow by reusing the ios-end-to-end-tests for generating xcresult. it's a great step towards enhancing efficiency and consistency in iOS test reporting.


This change is Reviewable

Copy link

linear bot commented Oct 18, 2024

@mojganii mojganii self-assigned this Oct 18, 2024
@mojganii mojganii added the iOS Issues related to iOS label Oct 18, 2024
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @mojganii)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 16 at r1 (raw file):

env:
  TEST_DEVICE_UDID: 00008130-0019181022F3803A
  OLD_APP_COMMIT_HASH: 895b7d98825e678f5d7023d5ea3c9b7beee89280

This commit doesn't seem to exist anymore
Do you have a link to a successful run of this flow that runs the tests correctly ?


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 62 at r1 (raw file):

      - uninstall_app
    uses: mullvad/mullvadvpn-app/.github/workflows/ios-end-to-end-tests.yml@main
    with:

I think we can remove the mullvad/mullvadvpn-app prefix here

Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 8 of 8 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

Copy link
Collaborator Author

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 16 at r1 (raw file):

Previously, buggmagnet wrote…

This commit doesn't seem to exist anymore
Do you have a link to a successful run of this flow that runs the tests correctly ?

I checked out this commit to address the issue reported at IOS-866. Additionally, you can view the successful execution of the flow here.


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 62 at r1 (raw file):

Previously, buggmagnet wrote…

I think we can remove the mullvad/mullvadvpn-app prefix here

we can't indeed. it is part of syntax.

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 62 at r1 (raw file):

Previously, mojganii wrote…

we can't indeed. it is part of syntax.

I'm not sure I follow, the previous version had this line
uses: ./.github/actions/ios-end-to-end-tests

Are you saying that wasn't working ?

Copy link
Collaborator Author

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 62 at r1 (raw file):

Previously, buggmagnet wrote…

I'm not sure I follow, the previous version had this line
uses: ./.github/actions/ios-end-to-end-tests

Are you saying that wasn't working ?

they aren't equal. the previous one is ./.github/actions/ios-end-to-end-tests which is using the workflow in the checked out repo but mine is using workflow from main branch which mullvad/mullvadvpn-app/.github/workflows/ios-end-to-end-tests.yml@main

Copy link
Collaborator Author

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 62 at r1 (raw file):

Previously, mojganii wrote…

they aren't equal. the previous one is ./.github/actions/ios-end-to-end-tests which is using the workflow in the checked out repo but mine is using workflow from main branch which mullvad/mullvadvpn-app/.github/workflows/ios-end-to-end-tests.yml@main

You can find more info here.

@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch from e6a8ba5 to 1c030a5 Compare October 21, 2024 15:03
buggmagnet
buggmagnet previously approved these changes Oct 22, 2024
Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 7 of 8 files reviewed, 2 unresolved discussions (waiting on @mojganii and @rablador)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 16 at r1 (raw file):

Previously, mojganii wrote…

I checked out this commit to address the issue reported at IOS-866. Additionally, you can view the successful execution of the flow here.

My bad, it looks like I didn't have the commit in my git history.


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 62 at r1 (raw file):
Ah right, reusable workflows
The docs does specify the following

  • ./.github/workflows/{filename} for reusable workflows in the same repository.

Both are in our repo, but I think it's fine to leave it like this. After looking at it again, I think I prefer the explicit version. rather than the implicit one.

rablador
rablador previously approved these changes Oct 22, 2024
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

Copy link
Contributor

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 13 at r2 (raw file):

    # At midnight every day.
    # Notifications for scheduled workflows are sent to the user who last modified the cron
    # syntax in the workflow file. If you update this you must have notifications for

As discussed offline, let's reinstate this comment

@mojganii mojganii dismissed stale reviews from rablador and buggmagnet via cf92ed3 October 22, 2024 09:48
@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch from 1c030a5 to cf92ed3 Compare October 22, 2024 09:48
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

Copy link
Collaborator Author

@mojganii mojganii left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)


.github/workflows/ios-end-to-end-tests-settings-migration.yml line 13 at r2 (raw file):

Previously, buggmagnet wrote…

As discussed offline, let's reinstate this comment

Done.

@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch from cf92ed3 to 1521f28 Compare October 22, 2024 15:02
rablador
rablador previously approved these changes Oct 23, 2024
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch from c5a6bfc to 1521f28 Compare October 23, 2024 07:52
Copy link
Contributor

@rablador rablador left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet and @mojganii)

@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch 4 times, most recently from 1b61729 to 24c443c Compare October 23, 2024 22:09
@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch 7 times, most recently from 46a9a49 to 0bbbd27 Compare October 24, 2024 19:43
@mojganii mojganii marked this pull request as ready for review October 24, 2024 20:02
@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch from 87d4fc1 to b73d5b5 Compare October 24, 2024 20:14
@mojganii mojganii marked this pull request as draft October 28, 2024 20:24
@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch 8 times, most recently from 9fb6588 to 938780b Compare October 29, 2024 19:35
@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch 5 times, most recently from 20aa103 to 8732873 Compare October 30, 2024 19:17
@mojganii mojganii force-pushed the convert-junit-output-artifacts-to-xcresult-ios-870 branch from bc8dac4 to b6844f0 Compare November 15, 2024 16:12
@mojganii
Copy link
Collaborator Author

It has been agreed that due to the high maintenance required for the workflow, it is better to execute it locally rather than in CI.

@mojganii mojganii closed this Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants