Skip to content

Conversation

jaeopt
Copy link
Contributor

@jaeopt jaeopt commented Apr 11, 2023

Summary

Add more support for sendOdpEvent error handling:

  • empty string for action: throws odpInvalidAction
  • nil/empty for type: use default "fullstack" type
  • replace identifiers alias (fs-user-id/FS-USER-ID/FS_USER_ID) with fs_user_id

Include other clean-ups:

  • upgrade macOS (12), xcode(14.3), and simulators (15.5 and 16.1) in github actions (tests hangs with old versions).
  • upgrade local tests (Scrtips/test_all.sh) to cover a range of iOS versions (github actions not good for full coverage).

Test plan

  • Add unit tests covering all cases

Issues

@jaeopt jaeopt requested a review from a team as a code owner April 11, 2023 17:30
@jaeopt jaeopt requested a review from yasirfolio3 April 11, 2023 17:31
yasirfolio3
yasirfolio3 previously approved these changes Apr 11, 2023
Copy link
Contributor

@yasirfolio3 yasirfolio3 left a comment

Choose a reason for hiding this comment

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

LGTM

@jaeopt jaeopt changed the title [FSSDK-9062] Jae/empty odp action [FSSDK-9062] add more support for sendOdpEvent error handling Apr 11, 2023
@jaeopt jaeopt requested a review from yasirfolio3 April 19, 2023 00:55
Copy link
Contributor

@yasirfolio3 yasirfolio3 left a comment

Choose a reason for hiding this comment

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

Looks good overall, just some minor changes suggested.

simulator_xcode_version: 12.4
- os: 13.7
simulator_xcode_version: 13.4.1
- os: 15.5
Copy link
Contributor

Choose a reason for hiding this comment

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

can we test os 14 here? since we're already testing 15 above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

github actions are terrible in supporting iOS. 15.5 and 16 are only available to macOS 12, which I need to workaround the hang issue. I'll try to extend the range when they get available.

if: "${{ github.event.inputs.PREP == '' && github.event.inputs.RELEASE == '' }}"
uses: optimizely/swift-sdk/.github/workflows/unit_tests.yml@master
##uses: optimizely/swift-sdk/.github/workflows/unit_tests.yml@master
uses: optimizely/swift-sdk/.github/workflows/unit_tests.yml@jae/empty-odp-action
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good overall, can we update to master before merging

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update it with the next PR coming :)

Comment on lines -81 to +84
# > ls /Applications/
##ls /Applications/
# - to find supported simulator os versions, run this (and find simulator with non-error "datapath")
# > xcrun simctl list --json devices
##xcrun simctl list --json devices
Copy link
Contributor

Choose a reason for hiding this comment

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

are these commands no longer required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're just for debugging to check available iOS simulators etc installed in github actions VM.

@jaeopt jaeopt merged commit 8148e32 into master Apr 19, 2023
@jaeopt jaeopt deleted the jae/empty-odp-action branch April 19, 2023 01:07
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.

2 participants