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

ci: Add testing on macOS #242

Closed
wants to merge 3 commits into from
Closed

Conversation

better0fdead
Copy link

Added macOS testing into ci.

  • [not needed] Tests
  • [not needed] Changelog
  • [not needed] Documentation

Closes #157

Copy link
Collaborator

@oleg-jukovec oleg-jukovec left a comment

Choose a reason for hiding this comment

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

Thank you for the patch, but we need to fix tests before merge. Could you do it?

ci: Add testing on macOS -> ci: add testing on macOS.

Comment on lines 218 to 219
- macos-10.15
- macos-11.0
Copy link
Collaborator

@oleg-jukovec oleg-jukovec Nov 29, 2022

Choose a reason for hiding this comment

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

Here we can limit to one macos version (a latest one?).

Copy link
Member

Choose a reason for hiding this comment

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

Moreover, 10.15 is going to EOL in GitHub Actions. And macos-12 (not macos-12.0!) appears now. Please, catch recent updates from https://github.com/tarantool/smtp/commits/master/.github/workflows/testing.yml.

Copy link
Author

Choose a reason for hiding this comment

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

According to tarantool.io tarantool supports 11, 12 versions of macOS. Changed according to that information.

Comment on lines 134 to 138
ref: refs/pull/${{ github.event.pull_request.number }}/merge

- name: Clone the connector
if: github.event_name != 'pull_request_target'
uses: actions/checkout@v2
Copy link
Collaborator

@oleg-jukovec oleg-jukovec Nov 29, 2022

Choose a reason for hiding this comment

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

The change looks like a rebase artifact. Please, rebase to a master branch manually.

Comment on lines +222 to +225
- 1.10.10
- 2.8.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there no way to use pre-build packages on MacOS?

Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking on it in background. Of course, it is possible and there are different ways with cons and pros. We should setup some infrastructure and support it in setup-tarantool. Reach me if you want to participate and/or push this activity.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, building Tarantool from sources for module CI seems painful. I think we must get rid of it if possible.

Copy link
Author

Choose a reason for hiding this comment

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

Maybe cache will resolve the problem. You can check it here macOS 12 Tarantool 2.8.1 test.

Copy link
Member

@DifferentialOrange DifferentialOrange left a comment

Choose a reason for hiding this comment

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

Does the whole "source install" thing was inspired by some existing CI/CD pipeline?

if: matrix.tarantool != 'brew' && steps.cache.outputs.cache-hit != 'true'

- name: Clone tarantool ${{ env.T_VERSION }}
uses: actions/checkout@v2
Copy link
Member

Choose a reason for hiding this comment

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

checkout@v2 uses node 12 which will be deprecated soon. It's better to migrate to v3 already.

Copy link
Author

@better0fdead better0fdead Nov 30, 2022

Choose a reason for hiding this comment

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

Updated

Comment on lines +222 to +225
- 1.10.10
- 2.8.1
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, building Tarantool from sources for module CI seems painful. I think we must get rid of it if possible.

@better0fdead better0fdead force-pushed the better0fdead/gh-157-macos branch 2 times, most recently from 48deb03 to 61c6467 Compare November 30, 2022 11:52
@better0fdead better0fdead marked this pull request as draft November 30, 2022 13:11
@better0fdead
Copy link
Author

Thank you for the patch, but we need to fix tests before merge. Could you do it?

ci: Add testing on macOS -> ci: add testing on macOS.

I suggest someone to take a look into it, since I am not very familiar with queue and it will probably take many time. Also I will be busy doing other tasks. If time is not very important I will try to fix.

@@ -782,7 +782,7 @@ func TestUtube_Put(t *testing.T) {
return
}

time.Sleep(2 * time.Second)
time.Sleep(2*time.Second + 30*time.Millisecond)
Copy link
Member

Choose a reason for hiding this comment

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

Such approach is definitely not good.

Copy link
Collaborator

@oleg-jukovec oleg-jukovec Dec 7, 2022

Choose a reason for hiding this comment

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

I'll try to explain the problem:

A situation that is solved by increasing the waiting time by a bit looks like a data race. Such a fix should be done only if you are absolutely sure that this is the expected behavior, and not a data race (and it is advisable to add a comment on this).

Copy link
Author

Choose a reason for hiding this comment

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

I understand it, it was done for experimentational purposes on github runners. I will not use it or give a full explanation for it.

Added macOS testing into ci.
Updated actions to versions with node16 runtime.

Closes #157
@DifferentialOrange
Copy link
Member

F

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.

Run tests on macOS
4 participants