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

Unifying test arguments and test workflows #503

Merged
merged 6 commits into from
Sep 16, 2021
Merged

Unifying test arguments and test workflows #503

merged 6 commits into from
Sep 16, 2021

Conversation

saratvemulapalli
Copy link
Member

@saratvemulapalli saratvemulapalli commented Sep 16, 2021

Description

  • Unifying test arguments across test suites and have a single runner via test.sh.
  • Restructured BWC tests to pull manifests from S3 and kick off the test suite.
  • Adding Jenkins job support for BWC tests

Issues Resolved

Closes #393 #492

Check List

  • 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: Sarat Vemulapalli <[email protected]>
Signed-off-by: Sarat Vemulapalli <[email protected]>
Signed-off-by: Sarat Vemulapalli <[email protected]>
Signed-off-by: Sarat Vemulapalli <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2021

Codecov Report

Merging #503 (c896849) into main (3a6dbae) will decrease coverage by 0.85%.
The diff coverage is 18.60%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #503      +/-   ##
==========================================
- Coverage   69.31%   68.45%   -0.86%     
==========================================
  Files          58       59       +1     
  Lines        1509     1525      +16     
==========================================
- Hits         1046     1044       -2     
- Misses        463      481      +18     
Impacted Files Coverage Δ
bundle-workflow/src/run_bwc_test.py 0.00% <0.00%> (ø)
bundle-workflow/src/run_integ_test.py 0.00% <0.00%> (ø)
bundle-workflow/src/test_workflow/test_args.py 0.00% <0.00%> (ø)
...kflow/src/test_workflow/bwc_test/bwc_test_suite.py 90.00% <100.00%> (-0.63%) ⬇️
...ow/src/ci_workflow/ci_check_gradle_dependencies.py 100.00% <0.00%> (ø)
...src/manifests_workflow/component_opensearch_min.py 100.00% <0.00%> (ø)

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 3a6dbae...c896849. Read the comment docs.

@saratvemulapalli saratvemulapalli changed the title Unifying test arguments and restructuring BWC tests to pull manifests Unifying test arguments and test workflows Sep 16, 2021
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Some nits, but feel free to defer to another PR, there's enough here.

By making bundle-workflow/src/run_integ_test.py and friends runnable it becomes rather difficult to test them (hence lack of tests for them today). You could refactor them into classes such as IntegTestSuiteRunner(TestSuite), sinking any common args logic into TestSuite and moving the runners into the test suite subfolders as well, and then calling .run. Then test.sh wouldn't need to do the switching of what test suite to run in bash and become a dumb caller for run.sh like the other runners.

export AWS_ROLE_ARN=arn:aws:iam::<AWS_JENKINS_ACCOUNT>:role/opensearch-test
export AWS_ROLE_SESSION_NAME=dummy-session

Next, configure temporary credentials in environment w/
Copy link
Member

Choose a reason for hiding this comment

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

Quote only the shell commands, this line should not be between ```.

@@ -34,6 +34,11 @@ pipeline {
name: 'test_run_id',
trim: true
),
string(
defaultValue: '',
name: 'test_suite_type',
Copy link
Member

Choose a reason for hiding this comment

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

Nit, test_suite_type -> test_suite, I think it's equally clear but shorter.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I like it. I have a follow up PR, will clean it up there.

| test_suite_type | Run a specific test suite. [integ-test, bwc-test] |
Copy link
Member

Choose a reason for hiding this comment

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

test_suite

@saratvemulapalli
Copy link
Member Author

saratvemulapalli commented Sep 16, 2021

Some nits, but feel free to defer to another PR, there's enough here.

By making bundle-workflow/src/run_integ_test.py and friends runnable it becomes rather difficult to test them (hence lack of tests for them today). You could refactor them into classes such as IntegTestSuiteRunner(TestSuite), sinking any common args logic into TestSuite and moving the runners into the test suite subfolders as well, and then calling .run. Then test.sh wouldn't need to do the switching of what test suite to run in bash and become a dumb caller for run.sh like the other runners.

Yeah absolutely, I didnt want to do major restructuring. Will take care of this in a follow up PR.
Issue: #505

args = TestArgs()
with TemporaryDirectory(keep=args.keep) as work_dir:
logging.info("Switching to temporary work_dir: " + work_dir)
os.chdir(work_dir)
Copy link
Member

Choose a reason for hiding this comment

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

Can use with WorkingDirectory(work_dir) as cur_dir here

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure will take care of this in a quick follow up PR.

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.

Unify invoking the test workflows
5 participants