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

Enable and fix race detector for CI #3096

Merged
merged 10 commits into from
Jul 19, 2023
Merged

Enable and fix race detector for CI #3096

merged 10 commits into from
Jul 19, 2023

Conversation

blakerouse
Copy link
Contributor

What does this PR do?

Enables the race detector for the CI unit tests including fixing the issues detected by enabling the race detector.

Why is it important?

Ensures that we do not have any data race conditions in the code.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • [ ] I have added an entry in ./changelog/fragments using the changelog tool
  • [ ] I have added an integration test or an E2E test

@blakerouse blakerouse added Team:Elastic-Agent Label for the Agent team skip-changelog backport-v8.8.0 Automated backport with mergify backport-v8.9.0 Automated backport with mergify labels Jul 18, 2023
@blakerouse blakerouse self-assigned this Jul 18, 2023
@blakerouse blakerouse requested a review from a team as a code owner July 18, 2023 20:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

This was referenced Jul 18, 2023
Closed
Closed
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-07-18T20:26:41.532+0000

  • Duration: 82 min 30 sec

Test stats 🧪

Test Results
Failed 0
Passed 11844
Skipped 58
Total 11902

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages.

  • run integration tests : Run the Elastic Agent Integration tests.

  • run end-to-end tests : Generate the packages and run the E2E Tests.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@elasticmachine
Copy link
Contributor

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 98.718% (77/78) 👍
Files 67.416% (180/267) 👍
Classes 66.329% (327/493) 👍
Methods 53.642% (1031/1922) 👍
Lines 39.904% (11778/29516) 👍 0.061
Conditionals 100.0% (0/0) 💚

@blakerouse
Copy link
Contributor Author

buildkite test this

@pierrehilbert pierrehilbert removed the backport-v8.8.0 Automated backport with mergify label Jul 19, 2023
ycombinator
ycombinator reviewed Jul 19, 2023
View reviewed changes
internal/pkg/agent/application/dispatcher/dispatcher_test.go
case <-time.After(200 * time.Microsecond):
// we're not expecting any reset,
case <-calledCh:
// Handle was called, expected
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice replacement of assert.Eventually.

ycombinator
ycombinator reviewed Jul 19, 2023
View reviewed changes
internal/pkg/agent/application/dispatcher/dispatcher_test.go
// we're not expecting any reset,
case <-calledCh:
// Handle was called, expected
case <-time.After(1 * time.Second):
Copy link
Contributor

@ycombinator ycombinator Jul 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: You could probably make this even shorter, given that the assert.Eventually before had a timeout of 100*time.Millisecond?

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 just like to keep it longer because it returns as soon as it happens. This is just to be sure that in the case that runner is slow that we don't hit that timeout just because of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You okay with me leaving it as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, OK to leave as-is.

ycombinator
ycombinator approved these changes Jul 19, 2023
View reviewed changes
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

LGTM.

@blakerouse blakerouse merged commit f31ceaa into elastic:main Jul 19, 2023
14 checks passed
@blakerouse blakerouse deleted the fix-race-detector branch July 19, 2023 20:54
mergify bot pushed a commit that referenced this pull request Jul 19, 2023
@blakerouse @mergify
Enable and fix race detector for CI (#3096)
755fde7 
* Stop ignoring RACE_DETECTOR when not on amd64.

The race detector is now supported on arm64 and aarch64.

* Enable the race detector in CI.

* Remove pkg/errors from gotest.go.

* Fix unchecked error return value.

* Fix tests.

---------

Co-authored-by: Craig MacKenzie <[email protected]>
Co-authored-by: Fae Charlton <[email protected]>
(cherry picked from commit f31ceaa)
@mergify mergify bot mentioned this pull request Jul 19, 2023
Merged
blakerouse added a commit that referenced this pull request Jul 20, 2023
@mergify @blakerouse
Enable and fix race detector for CI (#3096) (#3104)
d51e244 
* Stop ignoring RACE_DETECTOR when not on amd64.

The race detector is now supported on arm64 and aarch64.

* Enable the race detector in CI.

* Remove pkg/errors from gotest.go.

* Fix unchecked error return value.

* Fix tests.

---------

Co-authored-by: Craig MacKenzie <[email protected]>
Co-authored-by: Fae Charlton <[email protected]>
(cherry picked from commit f31ceaa)

Co-authored-by: Blake Rouse <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ycombinator ycombinator ycombinator approved these changes

@AndersonQ AndersonQ Awaiting requested review from AndersonQ AndersonQ is a code owner automatically assigned from elastic/elastic-agent-control-plane

@michalpristas michalpristas Awaiting requested review from michalpristas michalpristas is a code owner automatically assigned from elastic/elastic-agent-control-plane

@pchila pchila Awaiting requested review from pchila

Assignees

@blakerouse blakerouse

Labels
backport-v8.9.0 Automated backport with mergify skip-changelog Team:Elastic-Agent Label for the Agent team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@blakerouse @elasticmachine @ycombinator @pierrehilbert @cmacknz @faec