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.

Sign up for GitHub

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

Jump to bottom

[SDK-3831] CI job for iOS 17 #1802

Closed
wants to merge 11 commits into from
Closed

[SDK-3831] CI job for iOS 17 #1802

wants to merge 11 commits into from

Conversation

maratal
Copy link
Collaborator

@maratal maratal commented Sep 15, 2023 •
edited
Loading

Closes #1796

On top of #1801.

What's interesting, even while just co-existing (in this PR) with a workflow, containing macOS 13 (added iOS 17), some of those tests might fail.

So my suggestions to handle this would be one of the following:

  1. Skip those tests (and create an issue) until macOS 13 will be the default version and see what will happen.
  2. Increase ttl (bad idea, because it will just hide the problem).
  3. Rewrite tests not to use token authentication when possible (bad the same way as 2).

WDYT @lawrence-forooghian

Previous attemt with increased ttl.

@github-actions github-actions bot temporarily deployed to staging/pull/1802/features September 15, 2023 12:06 Inactive
@maratal maratal mentioned this pull request Sep 15, 2023
Closed
@github-actions github-actions bot temporarily deployed to staging/pull/1802/jazzydoc September 15, 2023 12:11 Inactive
@maratal
Now it fails on CI with 'XCTAssertEqual failed: ("Optional("patch")")…
f2942ec 
… is not equal to ("Optional("PATCH")")' vs failing locally before commit 028cde0 with

'XCTAssertEqual failed: ("Optional("PATCH")") is not equal to ("Optional("patch")")'. Just forcing uppercased then.
@github-actions github-actions bot temporarily deployed to staging/pull/1802/features September 15, 2023 14:22 Inactive
@maratal maratal mentioned this pull request Sep 15, 2023
Merged
@github-actions github-actions bot temporarily deployed to staging/pull/1802/jazzydoc September 15, 2023 14:30 Inactive
@lawrence-forooghian lawrence-forooghian changed the base branch from main to fix/1764-preparation-for-macos-13 September 19, 2023 12:28
@lawrence-forooghian
Copy link
Collaborator

lawrence-forooghian commented Sep 19, 2023 •
edited
Loading

On top of #1801.

When you have a PR that's based on top of an another one, the "into" branch of the second PR should be the branch of the first PR. I've changed it in this PR.

@lawrence-forooghian
Copy link
Collaborator

@maratal I think now that iOS / tvOS 17 are now released, we can just change the existing CI jobs to use them (and update the documentation in the README), instead of adding new ones. What do you think?

lawrence-forooghian
lawrence-forooghian reviewed Sep 19, 2023
View reviewed changes
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian left a comment

Choose a reason for hiding this comment

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

@maratal please could you give an explanation of what you believe is causing the five failures (i.e. ignoring the case-sensitivity one you've tried to fix in this PR) that you've linked to above? I don't think we can come to a decision without understanding this.

Test/Tests/RestClientTests.swift Outdated Show resolved Hide resolved
Base automatically changed from fix/1764-preparation-for-macos-13 to main September 19, 2023 15:21
@maratal
Copy link
Collaborator Author

maratal commented Sep 19, 2023 •
edited
Loading

@maratal I think now that iOS / tvOS 17 are now released, we can just change the existing CI jobs to use them (and update the documentation in the README), instead of adding new ones. What do you think?

But for that macos-13 should be selected as runner image OS (default is still macos-12). Which leads to all the failures described in #1801

@lawrence-forooghian
Copy link
Collaborator

But for that macos-13 should be selected as runner image OS (default is still macos-12). Which leads to all the failures described in #1801

I don't understand what you mean. What I'm saying is that instead of having a CI job for iOS 17 and also a CI job for iOS 16, we only need one for iOS 17 (since in general we only perform CI testing on the latest version of iOS). What is the connection between this and macOS version or test failures?

@github-actions github-actions bot temporarily deployed to staging/pull/1802/features September 20, 2023 14:09 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1802/jazzydoc September 20, 2023 14:14 Inactive
@maratal maratal force-pushed the fix/1796-CI-job-for-iOS-17 branch from 9e5024a to 8b4f1bf Compare September 20, 2023 14:18
@github-actions github-actions bot temporarily deployed to staging/pull/1802/features September 20, 2023 14:19 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1802/jazzydoc September 20, 2023 14:25 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1802/features September 20, 2023 23:39 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1802/jazzydoc September 20, 2023 23:44 Inactive
@maratal maratal mentioned this pull request Sep 25, 2023
Closed
@lawrence-forooghian
Copy link
Collaborator

@maratal are you sure that these test failures are dependent on iOS version? I've noticed that we seem to have some tests that fail intermittently. From what I've seen, it seems to depend on the CI runner — i.e. if the tests fail once on that runner, then they'll fail if you run them again (I've noticed this times when I've been running tests in a loop; some runners continuously fail and some don't fail at all). See here for examples, for example this one seems to match the list of tests you mentioned above.

Is it possibly something to do with the clock on the runner?

@maratal
Copy link
Collaborator Author

maratal commented Nov 14, 2023

@maratal are you sure that these test failures are dependent on iOS version? I've noticed that we seem to have some tests that fail intermittently. From what I've seen, it seems to depend on the CI runner — i.e. if the tests fail once on that runner, then they'll fail if you run them again (I've noticed this times when I've been running tests in a loop; some runners continuously fail and some don't fail at all). See here for examples, for example this one seems to match the list of tests you mentioned above.

Not iOS, macOS 13 back when I've created this PR. Recently I began to see those failures on macOS 12 as well from time to time. Now it happens a few days in a raw very often both on macOS 12 and 13.

Is it possibly something to do with the clock on the runner?

I don't know yet, but increasing token ttl helped to eliminate this. Do you see any improvement by adding tolerance to ttl?

@lawrence-forooghian
Copy link
Collaborator

Do you see any improvement by adding tolerance to ttl?

I made changes to specific tests for a specific reason; I don't think there's overlap with these tests.

@maratal
Merge remote-tracking branch 'origin/main' into fix/1796-CI-job-for-i…
c73a6ea 
…OS-17
@maratal
In a test suite this failure goes as "test__092__RestClient__request_…
d8502e7 
…_method_signature_and_arguments__should_do_a_request_and_receive_a_valid_response", i.e. the test which is being executed at the moment when fail("Completion closure should not be called") is called within "test__090__RestClient__..." test (https://github.com/ably/ably-cocoa/blob/ca4ffa1f582714945b9f323a1dc86b1da57c974f/Test/Tests/RestClientTests.swift#L1916).

Test passes frequently on CI because I assume tests failures after execution not always tracked by XCTest framework. Locally it crashes with this:
*** Terminating app due to uncaught exception 'NSInternalInconsistencyException', reason: 'Attempted to report a test failure to XCTest while no test case was running. The failure was:
"Completion closure should not be called".

The path "new feature" is not an invalid path, since it's being replaced with "new%20feature" in Foundation. So I'm removing this check.

I've noticed that this test fails frequently when other well known set of tests (https://test-observability.herokuapp.com/repos/ably/ably-cocoa/uploads/554d09f1-71e2-4e79-b641-9f33a8382220) don't fail. So when I've fixed it in this experimental PR (#1799), it finally could pass (after a few attemts).
@github-actions github-actions bot temporarily deployed to staging/pull/1802/features November 19, 2023 15:07 Inactive
@maratal maratal force-pushed the fix/1796-CI-job-for-iOS-17 branch from a1878f6 to 03c19d9 Compare November 19, 2023 15:10
@github-actions github-actions bot temporarily deployed to staging/pull/1802/features November 19, 2023 15:10 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1802/jazzydoc November 19, 2023 15:16 Inactive
@maratal maratal force-pushed the fix/1796-CI-job-for-iOS-17 branch from 03c19d9 to 1b1d213 Compare November 19, 2023 16:06
@maratal
Copy link
Collaborator Author

maratal commented Feb 5, 2024

Solved #1851

@maratal maratal closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lawrence-forooghian lawrence-forooghian lawrence-forooghian left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add CI job for latest Xcode 15 / iOS 17 and fix issues
2 participants
@maratal @lawrence-forooghian