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

Refactor BWC tests into sub project #359

Merged
merged 18 commits into from
Apr 11, 2022

Conversation

jmazanec15
Copy link
Member

Description

This PR clean up our build.gradle file. First, it refactors BWC tests into a subproject - :qa:bwc. This is how OpenSearch does it and will give independence between our integration test suite and BWC tests. In addition to this, I setup a BWC task to pull the OpenSeacrh release artifact and extract a zip of the k-NN plugin (addressing #320).

In order to share test code between the root project and the bwc project, I moved helper classes into a testfixtures source set. I would like to get feedback/thoughts on this. I would like to know if there is a better way to do this that I may not be thinking of?

I removed OSPackage code at the bottom of the main build.gradle file as it was no longer in used (we do not use it to build RPMs/DEBs).

Along with this, I changed the knncoverage to only apply to the rootproject.

Issues Resolved

#320

Check List

  • Commits are signed as per the DCO using --signoff

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.

Creates new sub project for BWC testing. This will allow us to
modularize the build files better.

Signed-off-by: John Mazanec <[email protected]>
@jmazanec15 jmazanec15 requested a review from a team April 7, 2022 20:12
@jmazanec15 jmazanec15 marked this pull request as draft April 7, 2022 20:12
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Apr 7, 2022

Codecov Report

Merging #359 (fc56754) into main (a8774ac) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main     #359   +/-   ##
=========================================
  Coverage     84.01%   84.01%           
  Complexity      911      911           
=========================================
  Files           130      130           
  Lines          3879     3879           
  Branches        359      359           
=========================================
  Hits           3259     3259           
  Misses          458      458           
  Partials        162      162           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8774ac...fc56754. Read the comment docs.

String knn_bwc_version = System.getProperty("bwc.version")

// Task to pull k-NN plugin from archive
task pullBwcPlugin {
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this section is unix os specific, can be a bummer in case we want to support something like win platform. did you try to find gradle native equivalent plugins/functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me take a look. For this specific project, given that we have only supportted Linux, I dont think it is blocking us. However, I will take a look for gradle tasks/functions that doe something similar.

Copy link
Member

Choose a reason for hiding this comment

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

sure, for us it works and isn't a blocker. it's more for customers on win or other non-unix like platforms. If it something that doesn't require a lot of efforts I think we need to do to that direction.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated for non-unix

@@ -4,6 +4,7 @@
#

version=1.0.0
systemProp.bwc.version=1.3.1
Copy link
Member

Choose a reason for hiding this comment

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

please make sure it's covered in plugin release guide

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update release guide

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@jmazanec15 jmazanec15 marked this pull request as ready for review April 8, 2022 15:25
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
Signed-off-by: John Mazanec <[email protected]>
@martin-gaievski
Copy link
Member

Looks good, there are multiple farther improvements possible, as discussed offline they are out of scope for this PR. Thank you!

@jmazanec15 jmazanec15 merged commit 3fe2abf into opensearch-project:main Apr 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants