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

Modify run_e2etest script to check for job_types and include_dirs #180

Merged
merged 16 commits into from
Jul 19, 2018
Merged

Modify run_e2etest script to check for job_types and include_dirs #180

merged 16 commits into from
Jul 19, 2018

Conversation

richardsliu
Copy link
Contributor

@richardsliu richardsliu commented Jul 17, 2018

Related to #150 run E2E tests selectively based on modified source files.
Related to #168 project velocity negatively impacted by test flakiness.

This change is Reviewable

@richardsliu
Copy link
Contributor Author

/uncc @jlewi
/uncc @gaocegege

@richardsliu richardsliu changed the title WIP Modify run_e2etest script Modify run_e2etest script to check for job_types and include_dirs Jul 17, 2018
@richardsliu
Copy link
Contributor Author

/assign @jlewi

@jlewi
Copy link
Contributor

jlewi commented Jul 18, 2018

This looks great.

Can you update README.md to provide instructions on how to scope tests? In particular can you include a snippet of what the update YAML looks like?

@richardsliu
Copy link
Contributor Author

Done.

README.md Outdated
component: workflows
name: unittests
job_types: presubmit
include_dirs: foo/*,bar/*
Copy link
Contributor

Choose a reason for hiding this comment

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

job_types and include_dirs should be arrays of strings e.g.

include_dirs:

  • foo/*
  • bar/*

@@ -72,8 +75,15 @@ def parse_config_file(config_file, root_dir):

components = []
for i in results["workflows"]:
job_types = []
if i.get("job_types"):
job_types = i.get("job_types").split(",")
Copy link
Contributor

Choose a reason for hiding this comment

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

job_types and include_dirs should be arrays.

@@ -105,7 +120,7 @@ def run(args, file_handler): # pylint: disable=too-many-statements,too-many-bran

if args.app_dir and args.component:
# TODO(jlewi): We can get rid of this branch once all repos are using a prow_config.xml file.
workflows.append(WorkflowComponent("legacy", args.app_dir, args.component, {}))
workflows.append(WorkflowComponent("legacy", args.app_dir, args.component, [], [], {}))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be safe to delete this now. We could also do this in a separate PR either before or after this PR. In the event we do have some tests not updated breaking them is one of the quickest ways to find them.

@richardsliu
Copy link
Contributor Author

Done. I also removed testMainPresubmit since it no longer applies.

@jlewi
Copy link
Contributor

jlewi commented Jul 19, 2018

This is fantastic.

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jlewi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit a61f638 into kubeflow:master Jul 19, 2018
@richardsliu richardsliu deleted the e2etest branch July 19, 2018 01:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants