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

add bwc tests & enable code coverage badge #184

Merged
merged 2 commits into from
May 17, 2022

Conversation

sbcd90
Copy link
Contributor

@sbcd90 sbcd90 commented May 14, 2022

Signed-off-by: Subhobrata Dey sbcd90@gmail.com

Description

this PR add bwc tests & enable code coverage badge for job-scheduler.

Issues Resolved

Add backwards compatibility tests for automation

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed 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.

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
@sbcd90 sbcd90 requested a review from a team May 14, 2022 01:11
@codecov-commenter
Copy link

codecov-commenter commented May 14, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@49f2c83). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main     #184   +/-   ##
=======================================
  Coverage        ?   53.19%           
  Complexity      ?       65           
=======================================
  Files           ?        8           
  Lines           ?      438           
  Branches        ?       50           
=======================================
  Hits            ?      233           
  Misses          ?      186           
  Partials        ?       19           

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 49f2c83...bab9886. Read the comment docs.

@sbcd90 sbcd90 requested review from downsrob and bowenlan-amzn May 14, 2022 02:01
.github/workflows/bwc-test-workflow.yml Outdated Show resolved Hide resolved
sample-extension-plugin/build.gradle Outdated Show resolved Hide resolved
break;
case UPGRADED:
Assert.assertTrue(pluginNames.contains("opensearch-job-scheduler"));
createBasicWatcherJob();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a comment about the strategy of the BWC test? It would be difficult for someone with no context to realize why you are using the hardcoded string with no explanation. I would also like to see some more comments throughout, I always find the BWC tests a bit difficult to read.

Copy link
Member

Choose a reason for hiding this comment

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

+1
I am wondering why only do the createBasicWatcherJob in UPGRADED but not OLD and MIXED.

Copy link
Member

Choose a reason for hiding this comment

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

From the gradle file bwc tests setup, it looks to me that the rolling upgrade bwc tests are using the same cluster testClusters."${baseName}0, so prob you only need to create job at the OLD cluster, and only assert it's running at MIXED and UPGRADED cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi @bowenlan-amzn , we cannot create a job in old cluster as sample-extension plugin is not loaded as old zip file is not available

Assert.assertTrue(pluginNames.contains("opendistro-job-scheduler"));
break;
case MIXED:
Assert.assertTrue(pluginNames.contains("opensearch-job-scheduler"));
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to assert both opendistro and opensearch here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed it

Signed-off-by: Subhobrata Dey <sbcd90@gmail.com>
@sbcd90 sbcd90 merged commit 2c4ed4a into opensearch-project:main May 17, 2022
wuychn pushed a commit to ochprince/job-scheduler that referenced this pull request Mar 16, 2023
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.

6 participants