require test manifest path for running tests #1229

tianleh · 2021-12-02T06:07:13Z

Signed-off-by: Tianle Huang tianleh@amazon.com

Description

Move test manifest to the manifests dir and require --test-manifest-path. This will address parts of @dblock 's comments on #1222

Issues Resolved

Resolves #1228

Test

local tests

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.

codecov-commenter · 2021-12-02T06:09:21Z

Codecov Report

Merging #1229 (dd26c3e) into main (3795a28) will increase coverage by 0.80%.
The diff coverage is 94.73%.

@@             Coverage Diff              @@
##               main    #1229      +/-   ##
============================================
+ Coverage     93.26%   94.06%   +0.80%     
  Complexity       11       11              
============================================
  Files           116      120       +4     
  Lines          2761     2799      +38     
  Branches         10       10              
============================================
+ Hits           2575     2633      +58     
+ Misses          176      156      -20     
  Partials         10       10

Impacted Files	Coverage Δ
src/ci_workflow/ci_manifest.py	`75.00% <75.00%> (ø)`
src/ci_workflow/ci_input_manifest.py	`100.00% <100.00%> (ø)`
src/ci_workflow/ci_manifests.py	`100.00% <100.00%> (ø)`
src/ci_workflow/ci_test_manifest.py	`100.00% <100.00%> (ø)`
src/run_ci.py	`90.00% <100.00%> (+4.28%)`	⬆️
src/run_integ_test.py	`92.50% <100.00%> (-0.19%)`	⬇️
src/test_workflow/test_args.py	`100.00% <100.00%> (ø)`
src/manifests/bundle/bundle_manifest_1_0.py	`90.62% <0.00%> (+19.65%)`	⬆️
src/manifests/build/build_manifest_1_0.py	`90.62% <0.00%> (+22.88%)`	⬆️
src/manifests/build/build_manifest_1_1.py	`90.62% <0.00%> (+22.88%)`	⬆️

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 3795a28...dd26c3e. Read the comment docs.

tianleh · 2021-12-02T08:21:39Z

Introduced an attribute type to TestManifest so that CI can have different validation logic for InputManifest vs TestManifest

dblock

~~I don't think I understand what purpose type serves when it can only be "test"? What other type will be there? When does it need to be distinguished from an input manifest?~~ Oh I see, CI is failing on these manifests because it doesn't know that it's a test manifest. For now I'd rather not add a type (we should then be adding types to all manifests, which seems like an overkill), but just hard-code skipping files that are called *-test.yml.
It seems like all test workflows require the test manifest. If that's the case it should be a required positional parameter, e.g../test.sh [kind of test] [test manifest] [build location] to be consistent with the other runners.

tianleh · 2021-12-02T18:29:03Z

For 1, sure. I can switch to a simple file name check.

For 2. This is already achieved by this PR. If not specified, this will fail
self.test_manifest_path = args.test_manifest_path if validators.url(args.test_manifest_path) else os.path.realpath(args.test_manifest_path)

dblock · 2021-12-02T19:02:55Z

For 2. This is already achieved by this PR. If not specified, this will fail self.test_manifest_path = args.test_manifest_path if validators.url(args.test_manifest_path) else os.path.realpath(args.test_manifest_path)

As a side effect - you get an error that the path specified doesn't exist or not a URL vs. you forgot to specify it. Make it required in test args. Then I would really change --arg to a positional operator, because it's always required to be consistent with all other runners.

tests/test_run_ci.py

src/run_ci.py

src/test_workflow/test_args.py

dblock

You're almost there!

Look at what's common between CiInputManifest and CiTestManifest: they both create an instance of a Manifest by calling .from_file on it and both take args. Add that into CiManifest constructor.

class CiManifest:
  def __init__(self, manifest, args):
    self.manifest = manifest
    self.args = args

Your child class becomes:

class CiInputManifest(CiManifest):
  def __init__(self, filename, args):
    super().__init__(self, InputManifest.from_file(filename), args)

The check method is on both classes and should be implemented on CiManifest for all the common parts. The pattern is to have a check method on the base class and an abstract __check__ method overridden in both child classes.

def check(self):
    try:
       self.__check__()
    except:
       ...
       raise

Finally, __is_test_manifest is glaring "not object oriented", but now that your CiManifest takes the same arguments it becomes easier, add a CiManifests class.

class CiManifests:
    def __klass(filename):
        if re.search(...):
            return CiTestManifest
        else:
            return CiInputManifest
   
    def from_file(filename, args):
        return self.__klass(filename)(filename, args)

dblock · 2021-12-03T14:10:03Z

jenkins/test/testsuite/Jenkinsfile

@@ -60,7 +60,7 @@ pipeline {
                script {
                    basePath = "https://ci.opensearch.org/ci/dbc/bundle-build/${opensearch_version}/${build_id}/${platform}/${architecture}"
                    sh "wget ${basePath}/builds/opensearch/manifest.yml"
-                    sh "./test.sh ${JOB_NAME} ${basePath} --test-run-id ${env.BUILD_NUMBER}"
+                    sh "./test.sh ${JOB_NAME} manifests/${opensearch_version}/opensearch-${opensearch_version}-test.yml ${basePath} --test-run-id ${env.BUILD_NUMBER}"
                }
            }
            post() {


For later: we should probably move this file under opensearch/ since we'll need one of these for Dashboards.

Dashboards will have a test manifest with "opensearch-dashboards". Thus sub dir is no needed due to different names.

src/ci_workflow/ci_test_manifest.py

tianleh · 2021-12-03T16:32:11Z

Thanks for the detailed instructions! The code now looks super elegant

tianleh · 2021-12-03T19:35:35Z

Noticed "This branch has conflicts that must be resolved". Taking a look

Signed-off-by: Tianle Huang <tianleh@amazon.com>

tianleh · 2021-12-03T20:23:52Z

Resolved merge conflict. Please help review. @dblock

dblock

Nice and clean!

tianleh requested a review from a team as a code owner December 2, 2021 06:07

tianleh requested review from dblock, seraphjiang, kavilla and peterzhuamazon December 2, 2021 06:10

tianleh self-assigned this Dec 2, 2021

seraphjiang approved these changes Dec 2, 2021

View reviewed changes

tianleh mentioned this pull request Dec 2, 2021

Support running OpenSearch or OpenSearch Dashboards integ test from t… #1222

Merged

1 task

tianleh added the ci-test-automation label Dec 2, 2021

dblock requested changes Dec 2, 2021

View reviewed changes

peternied reviewed Dec 2, 2021

View reviewed changes

tests/test_run_ci.py Show resolved Hide resolved

src/run_ci.py Outdated Show resolved Hide resolved

dblock reviewed Dec 2, 2021

View reviewed changes

src/run_ci.py Outdated Show resolved Hide resolved

dblock reviewed Dec 2, 2021

View reviewed changes

src/test_workflow/test_args.py Outdated Show resolved Hide resolved

tianleh requested a review from dblock December 3, 2021 01:06

dblock requested changes Dec 3, 2021

View reviewed changes

tianleh requested a review from dblock December 3, 2021 16:47

tianleh added 8 commits December 3, 2021 19:42

require test manifest path for running tests

e6cbed2

Signed-off-by: Tianle Huang <tianleh@amazon.com>

fix style

99fdb32

Signed-off-by: Tianle Huang <tianleh@amazon.com>

fix the CI manifest checks

886959d

Signed-off-by: Tianle Huang <tianleh@amazon.com>

address comments

22eea49

Signed-off-by: Tianle Huang <tianleh@amazon.com>

update README.md

97b8f5d

Signed-off-by: Tianle Huang <tianleh@amazon.com>

address feedback

1201a89

Signed-off-by: Tianle Huang <tianleh@amazon.com>

fix import

0bfc2ae

Signed-off-by: Tianle Huang <tianleh@amazon.com>

refactor CI manifests

ae7c2ba

Signed-off-by: Tianle Huang <tianleh@amazon.com>

tianleh added 3 commits December 3, 2021 19:56

update readme

4bf768f

Signed-off-by: Tianle Huang <tianleh@amazon.com>

more docs

07e09dc

Signed-off-by: Tianle Huang <tianleh@amazon.com>

remove type

dd26c3e

Signed-off-by: Tianle Huang <tianleh@amazon.com>

tianleh force-pushed the move-test-manifest branch from cf8ab75 to dd26c3e Compare December 3, 2021 20:02

dblock approved these changes Dec 3, 2021

View reviewed changes

dblock merged commit 86f9cbe into opensearch-project:main Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

require test manifest path for running tests #1229

require test manifest path for running tests #1229

tianleh commented Dec 2, 2021 •

edited

Loading

codecov-commenter commented Dec 2, 2021 •

edited

Loading

tianleh commented Dec 2, 2021

dblock left a comment •

edited

Loading

tianleh commented Dec 2, 2021

dblock commented Dec 2, 2021

dblock left a comment •

edited

Loading

dblock Dec 3, 2021

tianleh Dec 3, 2021

tianleh commented Dec 3, 2021

tianleh commented Dec 3, 2021

tianleh commented Dec 3, 2021

dblock left a comment

require test manifest path for running tests #1229

require test manifest path for running tests #1229

Conversation

tianleh commented Dec 2, 2021 • edited Loading

Description

Issues Resolved

Test

Check List

codecov-commenter commented Dec 2, 2021 • edited Loading

Codecov Report

tianleh commented Dec 2, 2021

dblock left a comment • edited Loading

Choose a reason for hiding this comment

tianleh commented Dec 2, 2021

dblock commented Dec 2, 2021

dblock left a comment • edited Loading

Choose a reason for hiding this comment

dblock Dec 3, 2021

Choose a reason for hiding this comment

tianleh Dec 3, 2021

Choose a reason for hiding this comment

tianleh commented Dec 3, 2021

tianleh commented Dec 3, 2021

tianleh commented Dec 3, 2021

dblock left a comment

Choose a reason for hiding this comment

tianleh commented Dec 2, 2021 •

edited

Loading

codecov-commenter commented Dec 2, 2021 •

edited

Loading

dblock left a comment •

edited

Loading

dblock left a comment •

edited

Loading