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

internal/civisibility: intelligent test runner support #2943

Merged
merged 37 commits into from
Oct 29, 2024

Conversation

tonyredondo
Copy link
Member

@tonyredondo tonyredondo commented Oct 23, 2024

What does this PR do?

This PR adds the Test Visibility Intelligent Test Runner feature: https://datadoghq.atlassian.net/wiki/spaces/SDTEST/pages/3123348305/Intelligent+Test+Runner+in+CI+Visibility+Libraries

Motivation

This is one of the most important feature for Test Visibility.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@tonyredondo tonyredondo force-pushed the tony/civisibility-test-code-coverage-research branch 2 times, most recently from 672e27b to 6ccdc75 Compare October 24, 2024 10:35
@DataDog DataDog deleted a comment from pr-commenter bot Oct 24, 2024
@pr-commenter
Copy link

pr-commenter bot commented Oct 24, 2024

Benchmarks

Benchmark execution time: 2024-10-29 10:39:35

Comparing candidate commit 17e359b in PR branch tony/civisibility-test-code-coverage-research with baseline commit 4b56ee5 in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 59 metrics, 0 unstable metrics.

@tonyredondo tonyredondo marked this pull request as ready for review October 24, 2024 15:23
@tonyredondo tonyredondo requested a review from a team as a code owner October 24, 2024 15:23
@tonyredondo tonyredondo force-pushed the tony/civisibility-test-code-coverage-research branch from 93d3a97 to 2dcc3fc Compare October 24, 2024 16:15
Copy link
Contributor

@ManuelPalenzuelaDD ManuelPalenzuelaDD left a comment

Choose a reason for hiding this comment

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

LGTM. I lack context in a lot of the implementation details of this, but the go impl looks ok. I've left some minor Qs/nits

}

// Create a dummy event to send with the coverage payload.
dummyEvent := FormFile{
Copy link
Contributor

Choose a reason for hiding this comment

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

Q: Why do we need to upload a dummy event?

Copy link
Member Author

Choose a reason for hiding this comment

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

}

restore := setStdOutToTemp()
t.postCoverageFilename = filepath.Join(temporaryDir, fmt.Sprintf("%d-%d-%d-post.out", t.moduleID, t.suiteID, t.testID))
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this line inside the restore := setStdOutToTemp() block? isn't it only necessary for the tearDown fn to be in that block?

Copy link
Member Author

Choose a reason for hiding this comment

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

True, I did a refactor to remove that method and just create a tearDown wrapper in 1ba2dda

@tonyredondo tonyredondo force-pushed the tony/civisibility-test-code-coverage-research branch from 1ba2dda to 396f778 Compare October 28, 2024 22:11
…client library. Also adds a new method to send coverage data.
@tonyredondo tonyredondo force-pushed the tony/civisibility-test-code-coverage-research branch from 17e359b to 3ccf36e Compare October 29, 2024 11:41
@tonyredondo tonyredondo merged commit fcda5e2 into main Oct 29, 2024
149 of 150 checks passed
@tonyredondo tonyredondo deleted the tony/civisibility-test-code-coverage-research branch October 29, 2024 12:06
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