Separated out tests in a different GHA workflow #84

owaiskazi19 · 2023-10-11T22:35:13Z

Description

Separated out test, integTest and yamRestTest in a different GHA workflow

Issues Resolved

Part of #73

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.

Signed-off-by: Owais Kazi <[email protected]>

dbwiddis

LGTM.

See also this link for a potential fix for the flaky integ test.

dbwiddis · 2023-10-12T16:10:18Z

.github/workflows/CI.yml

      run: |
-          ./gradlew check
+          ./gradlew check -x test -x integTest -x yamlRestTest
    - name: Upload Coverage Report
      if: matrix.os == 'ubuntu-latest'
      uses: codecov/codecov-action@v3


Actually... codecov may depend on the tests being run. Does this still work?

It didn't run on this PR. That's something to think about. Do you think I should try moving codecov to tests.yml?

Maybe. I'm still not clear on the motivation to split everything out from "check" which did it all anyway.

Also since each step in the tests.yml does its own checkout, I'm not sure if the coverage would be cumulative for all the steps.

Yaay! Codecov worked!

The motivation behind is to run the tests and build in parallel and reduce the CI time overall when our plugin will become more heavy.

run the tests and build in parallel

Hmm.

Full gradle check gives a total time of 50.570s.
Gradle check without the tests takes 9.819s.
Running the tests separately gives 42.721s, because you're still running them sequentially.

You could get them to run in parallel by putting them up under the highest level jobs: label.

Task Duration

:integTest 14.761s

:test 14.051s

:yamlRestTest 10.518s

Note jacoco is only considering unit tests, so you could (for now) put jacoco in that job and run the other two jobs without jacoco.

jacocoTestReport { dependsOn test reports { xml.required = true } }

There are ways to get jacoco to combine others: https://docs.gradle.org/7.4-rc-1/userguide/test_report_aggregation_plugin.html but I'm not sure that's needed.

codecov questions

Signed-off-by: Owais Kazi <[email protected]>

codecov · 2023-10-12T23:51:38Z

Codecov Report

Merging #84 (dad496b) into main (014475d) will increase coverage by 0.17%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main      #84      +/-   ##
============================================
+ Coverage     81.68%   81.85%   +0.17%     
- Complexity      285      287       +2     
============================================
  Files            30       30              
  Lines          1119     1119              
  Branches        125      125              
============================================
+ Hits            914      916       +2     
  Misses          160      160              
+ Partials         45       43       -2

see 2 files with indirect coverage changes

dbwiddis · 2023-10-13T03:16:27Z

.github/workflows/test.yml

+  test:
+    strategy:
+      matrix:
+        os:
+          - ubuntu-latest
+          - macOS-latest
+          - windows-latest
+        java:
+          - 17
+    if: github.repository == 'opensearch-project/opensearch-ai-flow-framework'
+    runs-on: ${{ matrix.os }}
+    steps:
+      - uses: actions/checkout@v4
+
+      - name: Set up JDK ${{ matrix.java }}
+        uses: actions/setup-java@v3
+        with:
+          java-version: ${{ matrix.java }}
+          distribution: temurin
+
+      - name: Test
+        run: ./gradlew test
+
+      - name: YamlRestTest
+        run:
+          ./gradlew yamlRestTest
+
+      - name: integTest
+        run:
+          ./gradlew integTest
+
+      - name: Run Tests with Coverage
+        run:
+          ./gradlew test jacocoTestReport --info
+
+      - name: Upload Coverage Report
+        if: matrix.os == 'ubuntu-latest'
+        uses: codecov/codecov-action@v3
+        with:
+          file: ./build/reports/jacoco/test/jacocoTestReport.xml


Suggested change

test:

strategy:

matrix:

os:

- ubuntu-latest

- macOS-latest

- windows-latest

java:

- 17

if: github.repository == 'opensearch-project/opensearch-ai-flow-framework'

runs-on: ${{ matrix.os }}

steps:

- uses: actions/checkout@v4

- name: Set up JDK ${{ matrix.java }}

uses: actions/setup-java@v3

with:

java-version: ${{ matrix.java }}

distribution: temurin

- name: Test

run: ./gradlew test

- name: YamlRestTest

run:

./gradlew yamlRestTest

- name: integTest

run:

./gradlew integTest

- name: Run Tests with Coverage

run:

./gradlew test jacocoTestReport --info

- name: Upload Coverage Report

if: matrix.os == 'ubuntu-latest'

uses: codecov/codecov-action@v3

with:

file: ./build/reports/jacoco/test/jacocoTestReport.xml

test:

if: github.repository == 'opensearch-project/opensearch-ai-flow-framework'

strategy:

matrix:

test:

- test jacocoTestReport

- integTest

- yamlRestTest

os:

- ubuntu-latest

- macOS-latest

- windows-latest

java:

- 17

runs-on: ${{ matrix.os }}

steps:

- uses: actions/checkout@v4

- name: Set up JDK ${{ matrix.java }}

uses: actions/setup-java@v3

with:

java-version: ${{ matrix.java }}

distribution: temurin

- name: Test

run: ./gradlew ${{ matrix.test }}

- name: Upload Coverage Report

if: matrix.os == 'ubuntu-latest' && matrix.test == 'test jacocoTestReport'

uses: codecov/codecov-action@v3

with:

file: ./build/reports/jacoco/test/jacocoTestReport.xml

You could have raised the PR directly :)

It's probably not the right way to go, just pointing out how to achieve what you're trying to do :)

Signed-off-by: Owais Kazi <[email protected]>

amitgalitz · 2023-10-13T16:48:37Z

.github/workflows/CI.yml

-      uses: codecov/codecov-action@v3
-      with:
-        file: ./build/reports/jacoco/test/jacocoTestReport.xml
+          ./gradlew check -x test -x integTest -x yamlRestTest


just wondering, why not use gradlew build here?

We can. I don't have any strong opinion here. We had check before I kept the same.

I am good with either, ./gradlew build means we also run assemble which runs the :compileJava, :processResources which check does also but this does it into a jar.

Changed it to build.

I think we should step back and figure out all the dependencies and figure out what we are trying to optimize here. With the current PR we're shifting the tests somewhere else (still in series so not really gaining much) but duplicating compile/build stuff in parallel.

My general preference would be for CI to keep all the check stuff and test and code coverage bits, and just spin off the integ tests and yaml tests into another job in parallel (or just put them in the CI yaml at the top level under "jobs").

Lots of different ways to do it, haven't really been convinced we have a problem that needs solving yet :)

I agree with you. I will go ahead and close the PR. We can always come back and open it back when the plugin becomes heavy.

amitgalitz · 2023-10-13T16:54:19Z

.github/workflows/test.yml

+          - macOS-latest
+          - windows-latest
+        java:
+          - 17


I remember in AD we had to test on 11, 17, 20. Should we do that here too? I think default for 2.x is jdk 11 still but could be wrong

@dbwiddis WDYT? Since you worked on the CI initially.

https://opensearch.org/docs/latest/install-and-configure/install-opensearch/index/#java-compatibility this is what we have documented and i think java 20 is going to be for 3.x

Pushed the change.

We should probably match whatever OpenSearch tests.

amitgalitz · 2023-10-13T17:04:14Z

.github/workflows/test.yml

+        test:
+          - test jacocoTestReport
+          - integTest
+          - yamlRestTest


also just for understanding, what are yamlRestTests for?

It's for testing the rest-api-spec yaml files. We don't have right now for our plugin, but we would in the future.

Here's an example test https://github.com/opensearch-project/OpenSearch/blob/1447d75e5e9ea042dd4bb645d71f25aea1fb42a8/modules/geo/src/yamlRestTest/resources/rest-api-spec/test/geo_shape/10_basic.yml ... it tests a search API with specific input and makes sure it gets the expected output.

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 · 2023-10-13T18:01:43Z

Closing this PR, as the plugin is quite lightweight now and doesn't take much time to process. This PR has some good discussion for the CI. We can always revisit this later later.

Separated out tests in a different GHA workflow

632b450

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 requested review from dbwiddis, joshpalis, ohltyler, amitgalitz and jackiehanyang as code owners October 11, 2023 22:35

github-actions bot added the backport 2.x backport PRs to 2.x branch label Oct 11, 2023

dbwiddis previously approved these changes Oct 12, 2023

View reviewed changes

dbwiddis reviewed Oct 12, 2023

View reviewed changes

dbwiddis self-requested a review October 12, 2023 17:41

owaiskazi19 force-pushed the tests-ci branch from da18261 to b27eba4 Compare October 12, 2023 22:35

dbwiddis approved these changes Oct 12, 2023

View reviewed changes

Handled code coverage for the PR

72c8b98

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the tests-ci branch from b27eba4 to 72c8b98 Compare October 12, 2023 23:18

owaiskazi19 added 3 commits October 12, 2023 16:26

Testing without MacOS

a441fd5

Signed-off-by: Owais Kazi <[email protected]>

Merge branch 'main' into tests-ci

4b63fbf

Setting up JDK 17

d59127d

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the tests-ci branch from c5de4bb to d59127d Compare October 12, 2023 23:36

Added macOS back

0c594dc

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the tests-ci branch from 480d58e to 0c594dc Compare October 12, 2023 23:46

Fixed codecov

adf0352

Signed-off-by: Owais Kazi <[email protected]>

dbwiddis reviewed Oct 13, 2023

View reviewed changes

Addressed PR comments

ea0028d

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 force-pushed the tests-ci branch from ecde9b7 to ea0028d Compare October 13, 2023 04:35

Merge branch 'main' into tests-ci

f9817f5

amitgalitz reviewed Oct 13, 2023

View reviewed changes

Addressed PR comments

dad496b

Signed-off-by: Owais Kazi <[email protected]>

owaiskazi19 requested a review from amitgalitz October 13, 2023 17:20

owaiskazi19 closed this Oct 13, 2023

dbwiddis mentioned this pull request Oct 14, 2023

Run integ tests in parallel with build #94

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separated out tests in a different GHA workflow #84

Separated out tests in a different GHA workflow #84

owaiskazi19 commented Oct 11, 2023

dbwiddis left a comment

dbwiddis Oct 12, 2023

owaiskazi19 Oct 12, 2023

dbwiddis Oct 12, 2023

dbwiddis Oct 12, 2023

owaiskazi19 Oct 12, 2023

owaiskazi19 Oct 12, 2023

dbwiddis Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading

dbwiddis Oct 13, 2023

owaiskazi19 Oct 13, 2023 •

edited

Loading

dbwiddis Oct 13, 2023

amitgalitz Oct 13, 2023

owaiskazi19 Oct 13, 2023 •

edited

Loading

amitgalitz Oct 13, 2023

owaiskazi19 Oct 13, 2023

dbwiddis Oct 13, 2023

owaiskazi19 Oct 13, 2023

amitgalitz Oct 13, 2023

owaiskazi19 Oct 13, 2023

amitgalitz Oct 13, 2023

owaiskazi19 Oct 13, 2023

dbwiddis Oct 13, 2023

amitgalitz Oct 13, 2023

owaiskazi19 Oct 13, 2023

dbwiddis Oct 13, 2023

owaiskazi19 commented Oct 13, 2023

Separated out tests in a different GHA workflow #84

Separated out tests in a different GHA workflow #84

Conversation

owaiskazi19 commented Oct 11, 2023

Description

Issues Resolved

dbwiddis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbwiddis Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Oct 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

owaiskazi19 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 Oct 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owaiskazi19 commented Oct 13, 2023

dbwiddis Oct 13, 2023 •

edited

Loading

codecov bot commented Oct 12, 2023 •

edited

Loading

owaiskazi19 Oct 13, 2023 •

edited

Loading

owaiskazi19 Oct 13, 2023 •

edited

Loading