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

Clean up -x task exclusions in ci.yml #2913

Closed
nibix opened this issue Jun 28, 2023 · 5 comments
Closed

Clean up -x task exclusions in ci.yml #2913

nibix opened this issue Jun 28, 2023 · 5 comments
Assignees
@nibix
Labels
good first issue These are recommended starting points for newcomers looking to make their first contributions. help wanted Community contributions are especially encouraged for these issues. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@nibix
Copy link
Collaborator

nibix commented Jun 28, 2023

At the moment, ci.yml contains many gradle task invocations with long exclusion lists like this:

         build test -Dbuild.snapshot=false
          -x integrationTest
          -x spotlessCheck
          -x checkstyleMain
          -x checkstyleTest
          -x spotbugsMain

This is because the jacoco plugin enforces explicit incoming dependencies from all test tasks. This will however also cause the execution of these "through the backdoor" if only a single test task is supposed to be executed.

It would be nice if a solution could be found to avoid that.

See also the conversation in this PR: #2861 (comment)
@github-actions github-actions bot added the untriaged Require the attention of the repository maintainers and may need to be prioritized label Jun 28, 2023
@nibix nibix mentioned this issue Jun 28, 2023
Merged
3 tasks
@pawel-gudel-eliatra
Copy link
Contributor

pawel-gudel-eliatra commented Jun 28, 2023 •
edited
Loading

Right now code in #2861 have one task exclusion:

    - name: Build and Test
      uses: gradle/gradle-build-action@v2
      with:
        arguments: |
          ${{ matrix.gradle_task }} -Dbuild.snapshot=false
          -x test

JaCoCo plugin does not force anything. We need to exclude "test" task because we want to generate coverage report, which is done by "jacocoTestReport" task, which task has circular dependency on "test" task (If you start "test" it always starts "jacocoTestReport" in the end AND if you start "jacocoTestReport" it begins from "test") That's why our every test is also trigerring "test" task.

Dependency between "test" and "JacocoTestReport" was introduced in commit 03a224d16 by @peternied.

Simplest fix is to remove this line: tasks.jacocoTestReport.dependsOn(test) but this will break creating coverage from only starting "jacocoTestReport" task - how many people are using that? How this would disrupt devs workflow?

@pawel-gudel-eliatra
Copy link
Contributor

pawel-gudel-eliatra commented Jun 28, 2023 •
edited
Loading

I've removed unnecessary exclusions from "integration-tests" job in commit e48b74 which means that only exclusion in ci.yml file is -x test and we cannot remove that without further information.

@peternied
Copy link
Member

Simplest fix is to remove this line: tasks.jacocoTestReport.dependsOn(test) but this will break creating coverage from only starting "jacocoTestReport" task

👍 has my blessing

pawel-gudel-eliatra added a commit to pawel-gudel-eliatra/security that referenced this issue Jul 5, 2023
@pawel-gudel-eliatra
Removed dependency between "test" and "jacocoTestReport"
e2e1d10 
Closes issue opensearch-project#2913.

This will allow us to not to exclude the "test" job from every other test.

Unfortunately, this will break creating the coverage reports from only starting "jacocoTestReport" task in Gradle.
pawel-gudel-eliatra added a commit to pawel-gudel-eliatra/security that referenced this issue Jul 5, 2023
@pawel-gudel-eliatra
Removed dependency between "test" and "jacocoTestReport"
ec88d2b 
Closes issue opensearch-project#2913.

This will allow us to not to exclude the "test" job from every other test.

Unfortunately, this will break creating the coverage reports from only starting "jacocoTestReport" task in Gradle.

Signed-off-by: Pawel Gudel <[email protected]>
@pawel-gudel-eliatra pawel-gudel-eliatra mentioned this issue Jul 5, 2023
Merged
3 tasks
@stephen-crawford
Copy link
Contributor

[Triage] Thank you @nibix for filing this issue. Seems like removing the jacoco line is the preferred option for the time being. Marking this issue as help wanted and good first issue.

@stephen-crawford stephen-crawford added good first issue These are recommended starting points for newcomers looking to make their first contributions. help wanted Community contributions are especially encouraged for these issues. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jul 10, 2023
DarshitChanpura pushed a commit that referenced this issue Aug 17, 2023
@pawel-gudel-eliatra
[Enchancement] Removed dependency between "test" and "jacocoTestRepor…
32d3112 
…t" (#2935)

As a part of discussions in #2861 there's been determined that CI
workflow in Github Actions is excluding a lot of unnecessary task.
@nibix created #2913 to address this problem. Most of the unneeded tasks
has been already removed in #2861. This PR removes last existing part.
@peternied's confirmed removal of dependency in his
[comment](#2913 (comment)).

This change removes the dependency between "_jacocoTestReport_" and
"_test_" tasks. Specifically the dependency that forces to start
"_test_" task always when "_jacocoTestReport_" is called. This allows us
to always generate coverage report in the end of every task run in "CI"
workflow without having to exclude "_test_" task in every single one of
them. Unfortunately this will break functionality to create code
coverage by starting only "_jacocoTestReport_" task. It looks like this
was not used widely and was certainly not used in any of Github Actions
Workflows. This will not break creating coverage reports in the end of
"_test_" task.


Signed-off-by: Pawel Gudel <[email protected]>
@davidlago
Copy link

Closing after #2935 merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nibix nibix

Labels
good first issue These are recommended starting points for newcomers looking to make their first contributions. help wanted Community contributions are especially encouraged for these issues. triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@peternied @davidlago @nibix @stephen-crawford @pawel-gudel-eliatra