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

Run CI (tests) on MacOS #3528

Merged
merged 9 commits into from Jan 15, 2024
Merged

Run CI (tests) on MacOS #3528

merged 9 commits into from Jan 15, 2024

Conversation

joanlopez
Copy link
Contributor

@joanlopez joanlopez commented Jan 11, 2024

What?

It enables macos-latest for test-related (test.yml) CI workflows.

Why?

Ha, why not? Jokes aside, imho now that MacOS-related issues (#2578 and #1230) have been closed (fixed), and that it looks like MacOS runners have at the very least a performance equivalent to Windows runners' (in fact, test jobs seems to be taking less time in MacOS runners), I guess it's good idea to make the check of the good functioning of k6 on MacOS part of the CI checks.

Also, in the worst case we can disable it again, but why not giving it a try?

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

I haven't found any.

@codecov-commenter
Copy link

codecov-commenter commented Jan 11, 2024

Codecov Report

Attention: 17 lines in your changes are missing coverage. Please review.

Comparison is base (d814297) 72.54% compared to head (1a6d5cc) 72.46%.
Report is 5 commits behind head on master.

❗ Current head 1a6d5cc differs from pull request most recent head 8047e70. Consider uploading reports for the commit 8047e70 to get more accurate results

Files Patch % Lines
execution/scheduler.go 57.57% 7 Missing and 7 partials ⚠️
execution/controller.go 75.00% 1 Missing and 1 partial ⚠️
cmd/inspect.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3528      +/-   ##
==========================================
- Coverage   72.54%   72.46%   -0.08%     
==========================================
  Files         278      278              
  Lines       20891    20933      +42     
==========================================
+ Hits        15155    15170      +15     
- Misses       4773     4788      +15     
- Partials      963      975      +12     
Flag Coverage Δ
ubuntu 72.46% <70.17%> (-0.01%) ⬇️
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

js/runner_test.go Outdated Show resolved Hide resolved
@joanlopez joanlopez force-pushed the ci-enable-macos branch 2 times, most recently from f64e905 to 2814ddf Compare January 11, 2024 17:48
@joanlopez joanlopez marked this pull request as ready for review January 11, 2024 21:09
@joanlopez joanlopez requested a review from a team as a code owner January 11, 2024 21:09
@joanlopez joanlopez requested review from mstoykov and olegbespalov and removed request for a team January 11, 2024 21:09
curl -fsSLO "https://raw.githubusercontent.com/codecov/codecov-bash/${CODECOV_BASH_VERSION}/codecov"
echo "$CODECOV_BASH_SHA512SUM codecov" | sha512sum -c -
echo "$CODECOV_BASH_SHA512SUM codecov" | sha512sum -c -
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For some reason (I didn't fully get), without the extra space it doesn't work on MacOS, but with it, it works correctly in all platforms 🤷🏻

Copy link
Collaborator

Choose a reason for hiding this comment

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

I remember that we had issues with codecov (which later were resolved) and created the task, but one of the highlights was that codecov has the GH action. So maybe it's worth migrating to it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, nice! Thanks for letting me know, I could consider working on that right after, but I'd prefer to keep it for a separate PR, if that sounds good to you as well 🙏🏻

@@ -37,8 +37,8 @@ jobs:
go version
export GOMAXPROCS=2
args=("-p" "2" "-race")
# Run with less concurrency on Windows to minimize flakiness.
if [[ "${{ matrix.platform }}" == windows* ]]; then
# Run with less concurrency on Windows/MacOS to minimize flakiness.
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 tried luck without this, and there has been no way to get all tests passing. I guess next step will be to take a look at that flakiness and try to correct it, but for now I guess it's fine, as it has been for Windows so far.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The issue is that tests are flaky on all platforms, primarily GRPC-TLS related https://github.com/grafana/xk6-grpc/issues/39. Which ones do you constantly see?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, there's nothing new, as I think all (or most) of them are documented at #2144.

It's just that it seems that Windows and MacOS runners are way more likely to fail (it's probably a matter of their performance), so I decided to add MacOS into that exception we already had for Windows, to avoid adding more flakiness.

@codebien codebien mentioned this pull request Jan 15, 2024
5 tasks
@mstoykov mstoykov added this to the v0.49.0 milestone Jan 15, 2024
@mstoykov mstoykov merged commit f35e679 into grafana:master Jan 15, 2024
25 checks passed
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.

None yet

4 participants