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

Creating Issue Reports for Flaky Test Failures in Gradle Check #436

Merged
merged 2 commits into from
Jun 13, 2024

Conversation

prudhvigodithi
Copy link
Collaborator

Description

Gradle Check flaky test failures issue creation

Issues Resolved

Part of opensearch-project/OpenSearch#13950

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@prudhvigodithi
Copy link
Collaborator Author

Adding @dblock since a similar setup was done in past for code inside src/jenkins. I have added a package gradlecheck that has the code which can be used by the library gradleCheckFlakyTestIssueCreate.groovy and jenkinsfile can simply load and call this library. The only change is I did not add implements Serializable also unlike each class part of src/jenkins the code inside src/gradlecheck is used only only gradleCheckFlakyTestIssueCreate library, I have done this way to make sure the code is organized following OOP methods rather than just dumping all the lines under one library inside var/gradleCheckFlakyTestIssueCreate.

Having this now the unit tests I have used mockito-core for testing the code inside tests/gradlecheck and works fine (example ./gradlew test --tests=OpenSearchMetricsQueryTest), but when invoked with existing tests the old tests times out with the following error.

2024-06-11T06:44:05.566+0000 [DEBUG] [org.gradle.cache.internal.DefaultFileLockManager] Lock acquired on daemon addresses registry.
2024-06-11T06:44:05.566+0000 [DEBUG] [org.gradle.cache.internal.DefaultFileLockManager] Releasing lock on daemon addresses registry.
2024-06-11T06:44:05.567+0000 [DEBUG] [org.gradle.cache.internal.DefaultFileLockManager] Waiting to acquire shared lock on daemon addresses registry.
2024-06-11T06:44:05.567+0000 [DEBUG] [org.gradle.cache.internal.DefaultFileLockManager] Lock acquired on daemon addresses registry.
2024-06-11T06:44:05.567+0000 [DEBUG] [org.gradle.cache.internal.DefaultFileLockManager] Releasing lock on daemon addresses registry.

Just deleting the directory src/gradlecheck and running the existing test example ./gradlew clean test --tests=TestUploadTestResults -info -Ppipeline.stack.write=true again moves forward without any timeout error.

I'm wondering why just adding one extra package src/gradlecheck and testing this with existing test suite is not possible ? @gaiksaya @peterzhuamazon @rishabh6788 @getsaurabh02

Copy link

codecov bot commented Jun 11, 2024

Codecov Report

Attention: Patch coverage is 86.36364% with 18 lines in your changes missing coverage. Please review.

Project coverage is 86.84%. Comparing base (cfba629) to head (1da3059).

Current head 1da3059 differs from pull request most recent head 3024899

Please upload reports for the commit 3024899 to get more accurate results.

Files Patch % Lines
...adlecheck/FetchPostMergeFailedTestClassTest.groovy 69.23% 1 Missing and 3 partials ⚠️
...radlecheck/FetchPostMergeFailedTestNameTest.groovy 76.47% 1 Missing and 3 partials ⚠️
...dlecheck/FetchPostMergeTestGitReferenceTest.groovy 73.33% 1 Missing and 3 partials ⚠️
tests/gradlecheck/FetchTestPullRequestsTest.groovy 73.33% 1 Missing and 3 partials ⚠️
...c/gradlecheck/FetchPostMergeFailedTestClass.groovy 91.66% 0 Missing and 1 partial ⚠️
tests/gradlecheck/CreateMarkDownTableTest.groovy 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #436      +/-   ##
============================================
- Coverage     87.12%   86.84%   -0.28%     
- Complexity       31       60      +29     
============================================
  Files            88      100      +12     
  Lines           233      365     +132     
  Branches         12       25      +13     
============================================
+ Hits            203      317     +114     
- Misses           22       26       +4     
- Partials          8       22      +14     

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

@prudhvigodithi
Copy link
Collaborator Author

The CI checks are now green and library is tested as jenkins run, sample issues that are created https://github.com/prudhvigodithi/OpenSearch/issues/25 prudhvigodithi/OpenSearch#24.

@prudhvigodithi prudhvigodithi self-assigned this Jun 12, 2024
@rishabh6788
Copy link
Collaborator

The CI checks are now green and library is tested as jenkins run, sample issues that are created prudhvigodithi/OpenSearch#25 prudhvigodithi/OpenSearch#24.

I see that there are two comments, if I remember correctly wasn't it supposed to be just one comment that would keep getting updated or description?
Also, I feel the way data is being presented can be improved.

@prudhvigodithi
Copy link
Collaborator Author

I see that there are two comments, if I remember correctly wasn't it supposed to be just one comment that would keep getting updated or description?
Also, I feel the way data is being presented can be improved.

Ya I have removed the issueEdit option to test the default behaviour (comment on an open issue rather than editing the body) of the createGithubIssue library. I have added it back.

The markdown table is coming from the approval opensearch-project/OpenSearch#13950 (comment), this has all the information per commit, associated PR, jenkins build and failed tests.
We can always improve based on the feedback once we see this in action by start creating the issues.

@@ -22,7 +24,7 @@ void call(Map args = [:]) {
try {
withCredentials([usernamePassword(credentialsId: 'jenkins-github-bot-token', passwordVariable: 'GITHUB_TOKEN', usernameVariable: 'GITHUB_USER')]) {
def openIssue = sh(
script: "gh issue list --repo ${args.repoUrl} -S \"${args.issueTitle} in:title\" --label ${label} --json number --jq '.[0].number'",
script: "gh issue list --repo ${args.repoUrl} -S \"${args.issueTitle} in:title\" --label \"${label}\" --json number --jq '.[0].number'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

In case there are multiple issues returned then is this picking up the top result?

Copy link
Collaborator Author

@prudhvigodithi prudhvigodithi Jun 12, 2024

Choose a reason for hiding this comment

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

Yes Rishab that was the idea for this existing library createGithubIssue, the logic was with same title for all the AUTOCUT issues, there will be only one issue created for one scenario.


}
def getPostMergeFailedTestName(testName, gitReference) {
return new OpenSearchMetricsQuery(metricsUrl,awsAccessKey, awsSecretKey, awsSessionToken, script).fetchMetrics(getQuery(testName, gitReference))
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: missing space after comma.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added thanks

}

def fetchMetrics(String query) {
def response = script.sh(
Copy link
Collaborator

Choose a reason for hiding this comment

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

just for my knowledge, what is 'script.sh'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The script is a jenkins context and will be injected to the library during runtime. Without the jenkins context the native groovy does have the sh inbuit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Adding more to this, to run a script in groovy the ideal way to use is .execute(), but running .execute() in jenkins is blocked by the script security plugin, hence the ideal was is to use sh given by jenkins and for sh to work in libraries it needs jenkins context.

@rishabh6788
Copy link
Collaborator

One thought I have is once the workflow has created an issue, and someone works on it, may be updates the comment or description and strikes-through a few failing tests that have been fixed.
Now when the workflow runs once again, wouldn't it override the edits made by operator?
Are we assuming that operator would only be adding new comments on the issue and not editing the comment/description added by with workflow?

@prudhvigodithi
Copy link
Collaborator Author

One thought I have is once the workflow has created an issue, and someone works on it, may be updates the comment or description and strikes-through a few failing tests that have been fixed.
Now when the workflow runs once again, wouldn't it override the edits made by operator?
Are we assuming that operator would only be adding new comments on the issue and not editing the comment/description added by with workflow?

AFAIK only maintainers/admins's can edit the issue created by external users and I dont think one can edit the issue created by other user.

String awsSessionToken
def script

FetchPostMergeFailedTestClass(String metricsUrl, String awsAccessKey, String awsSecretKey, String awsSessionToken, def script) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

except markdown class, every class has the same constructor and get_query methods. Would it be better to have a parent/super class or interface, if supported in groovy class, and have these method defined there and other classes just inherit it and implement in its own way. Method like getPostMergeFailedTestClass can have a generic name and implemented in all the sub-classes.
Not a blocker for me.

Copy link
Collaborator Author

@prudhvigodithi prudhvigodithi Jun 12, 2024

Choose a reason for hiding this comment

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

That was my initial implementation rishab, check this commit which has the extends and super implementation, the jenkins pipeline works but this extends and super implementation ran into issues with running the tests (both old and new tests part of the PR) part of this repo, please check my comment here.

@prudhvigodithi
Copy link
Collaborator Author

prudhvigodithi commented Jun 12, 2024

FYI I have added a comment to an open issue jenkinsci/JenkinsPipelineUnit#595 (comment) on why we cant test the library directly gradleCheckFlakyTestChecker that has the external classes. Here for our case the gradleCheckFlakyTestChecker just uses the code part of src/gradlecheck, for all the code part of src/gradlecheck is tested and covered in tests/gradlecheck (for reference please check the CodeCov #436 (comment)).

Copy link
Collaborator

@rishabh6788 rishabh6788 left a comment

Choose a reason for hiding this comment

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

Approving assuming all search queries are working as expected.

@prudhvigodithi
Copy link
Collaborator Author

Thanks @rishabh6788 just pushed a commit change the repo from my fork to repoUrl: "https://github.com/opensearch-project/OpenSearch",.

@prudhvigodithi prudhvigodithi merged commit 2e95cdd into opensearch-project:main Jun 13, 2024
8 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 13, 2024
Signed-off-by: Prudhvi Godithi <pgodithi@amazon.com>
(cherry picked from commit 2e95cdd)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot
Copy link

The backport to 6.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-6.x 6.x
# Navigate to the new working tree
pushd ../.worktrees/backport-6.x
# Create a new branch
git switch --create backport/backport-436-to-6.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 2e95cddcdf42ea02f45025b9ff1941240d7ba82f
# Push it to GitHub
git push --set-upstream origin backport/backport-436-to-6.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-6.x

Then, create a pull request where the base branch is 6.x and the compare/head branch is backport/backport-436-to-6.x.

prudhvigodithi pushed a commit that referenced this pull request Jun 13, 2024
Signed-off-by: Prudhvi Godithi <pgodithi@amazon.com>
(cherry picked from commit 2e95cdd)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants