Skip to content

Commit

Permalink
Always use configured branches in run, profiling and continuous commands
Browse files Browse the repository at this point in the history
Fix behavior of run, profiling and continuous commands that were using
get_hash_from_master() instead of configured branches when no commit was
specified.

In particular when only one branch was configured and was not "master",
asv tried to use the "master" branch.

Add (and refactor) specific test.

Closes #364
  • Loading branch information
philpep committed Jun 8, 2016
1 parent 75d1fc0 commit b7e237c
Show file tree
Hide file tree
Showing 8 changed files with 48 additions and 66 deletions.
8 changes: 4 additions & 4 deletions asv/commands/continuous.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def setup_arguments(cls, subparsers):
parent of the tested commit.""")
parser.add_argument(
'branch', default=None,
help="""The commit/branch to test. By default, the master branch.""")
help="""The commit/branch to test. By default, the first configured branch.""")
common_args.add_factor(parser)
common_args.add_show_stderr(parser)
common_args.add_bench(parser)
Expand All @@ -57,9 +57,9 @@ def run(cls, conf, branch=None, base=None, factor=2.0, show_stderr=False, bench=
repo.pull()

if branch is None:
head = repo.get_hash_from_master()
else:
head = repo.get_hash_from_name(branch)
branch = conf.branches[0]
head = repo.get_hash_from_name(branch)

if base is None:
parent = repo.get_hash_from_parent(head)
else:
Expand Down
5 changes: 2 additions & 3 deletions asv/commands/profiling.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,9 +120,8 @@ def run(cls, conf, benchmark, revision=None, gui=None, output=None,

machine_name = Machine.get_unique_machine_name()
if revision is None:
commit_hash = repo.get_hash_from_master()
else:
commit_hash = repo.get_hash_from_name(revision)
revision = conf.branches[0]
commit_hash = repo.get_hash_from_name(revision)

profile_data = None
checked_out = set()
Expand Down
4 changes: 2 additions & 2 deletions asv/commands/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ def setup_arguments(cls, subparsers):
machine. 'ALL' will benchmark all commits in the project.
'EXISTING' will benchmark against all commits for which
there are existing benchmarks on any machine. By default,
will benchmark the head of the current master branch.""")
will benchmark the head each configured branches.""")
parser.add_argument(
"--steps", "-s", type=common_args.positive_int, default=None,
help="""Maximum number of steps to benchmark. This is
Expand Down Expand Up @@ -145,7 +145,7 @@ def run(cls, conf, range_spec=None, steps=None, bench=None, parallel=1,
repo.pull()

if range_spec is None:
commit_hashes = [repo.get_hash_from_master()]
commit_hashes = set([repo.get_hash_from_name(branch) for branch in conf.branches])
elif range_spec == 'EXISTING':
commit_hashes = get_existing_hashes(conf.results_dir)
elif range_spec == "NEW":
Expand Down
4 changes: 2 additions & 2 deletions asv/environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -487,12 +487,12 @@ def install_project(self, conf, repo, commit_hash=None):
Uninstalls any installed copy of the project first.
If no specific commit hash is given, one is chosen;
either a choice that already exist in wheel cache,
or current master branch in the repository.
or first current branch configured.
"""
if commit_hash is None:
commit_hash = self._cache.get_existing_commit_hash()
if commit_hash is None:
commit_hash = repo.get_hash_from_master()
commit_hash = repo.get_hash_from_name(conf.branches[0])

self.uninstall(conf.project)

Expand Down
2 changes: 2 additions & 0 deletions asv/plugins/git.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ def get_hashes_from_range(self, range_spec):
return output.strip().split()

def get_hash_from_name(self, name):
if name is None:
name = self.get_branch_name()
return self._run_git(['rev-parse', name],
dots=False).strip().split()[0]

Expand Down
2 changes: 2 additions & 0 deletions asv/plugins/mercurial.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,8 @@ def get_hashes_from_range(self, range_spec):
followfirst=True)]

def get_hash_from_name(self, name):
if name is None:
name = self.get_branch_name()
return self._repo.log(name)[0].node

def get_hash_from_parent(self, name):
Expand Down
6 changes: 0 additions & 6 deletions asv/repo.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,12 +115,6 @@ def get_hash_from_name(self, name):
"""
raise NotImplementedError()

def get_hash_from_master(self):
"""
Get the hash of the current master branch commit.
"""
return self.get_hash_from_name(self.get_branch_name())

def get_hash_from_parent(self, name):
"""
Checkout the parent of the currently checked out commit.
Expand Down
83 changes: 34 additions & 49 deletions test/test_workflow.py
Original file line number Diff line number Diff line change
Expand Up @@ -182,40 +182,7 @@ def test_find(capfd, basic_conf):
assert "Greatest regression found: {0}".format(regression_hash[:8]) in output


def _test_run_branches(tmpdir, dvcs, conf, machine_file, range_spec,
branches, initial_commit):
# Find the current head commits for each branch
commits = [initial_commit]
for branch in branches:
commits.append(dvcs.get_hash(branch))

# Run tests
tools.run_asv_with_conf(conf, 'run', range_spec, '--quick',
'--bench=time_secondary.track_value',
'--skip-existing-commits',
_machine_file=machine_file)

# Check that files for all commits expected were generated
envs = list(environment.get_environments(conf, None))
tool_name = envs[0].tool_name

expected = set(['machine.json'])
for commit in commits:
for psver in ['0.3.6', '0.3.7']:
expected.add('{0}-{1}-py{2[0]}.{2[1]}-colorama{3}-six.json'.format(
commit[:8], tool_name, sys.version_info, psver))

result_files = os.listdir(join(tmpdir, 'results_workflow', 'orangutan'))

if range_spec == 'NEW':
assert set(result_files) == expected
elif range_spec == 'ALL':
assert set(expected).difference(result_files) == set([])
else:
raise ValueError()


def test_run_new_all(basic_conf):
def test_run_spec(basic_conf):
tmpdir, local, conf, machine_file = basic_conf
conf.wheel_cache_size = 5

Expand All @@ -226,6 +193,8 @@ def test_run_new_all(basic_conf):
conf.repo = dvcs.path

initial_commit = dvcs.get_hash("master~1")
master_commit = dvcs.get_hash("master")
branch_commit = dvcs.get_hash("some-branch")
template_dir = os.path.join(tmpdir, "results_workflow_template")
results_dir = os.path.join(tmpdir, 'results_workflow')
tools.run_asv_with_conf(conf, 'run', initial_commit+"^!",
Expand All @@ -234,25 +203,41 @@ def test_run_new_all(basic_conf):
_machine_file=join(tmpdir, 'asv-machine.json'))
shutil.copytree(results_dir, template_dir)

def rollback():
def _test_run(range_spec, branches, expected_commits):
# Rollback initial results
shutil.rmtree(results_dir)
shutil.copytree(template_dir, results_dir)

# Without branches in config, should just use master
_test_run_branches(tmpdir, dvcs, conf, machine_file, 'NEW',
branches=['master'], initial_commit=initial_commit)
rollback()
args = ["run", "--quick", "--skip-existing-successful",
"--bench=time_secondary.track_value"]
if range_spec is not None:
args.append(range_spec)
conf.branches = branches
tools.run_asv_with_conf(conf, *args, _machine_file=machine_file)

# Check that files for all commits expected were generated
envs = list(environment.get_environments(conf, None))
tool_name = envs[0].tool_name

_test_run_branches(tmpdir, dvcs, conf, machine_file, 'ALL',
branches=['master'], initial_commit=initial_commit)
rollback()
expected = set(['machine.json'])
for commit in expected_commits:
for psver in ['0.3.6', '0.3.7']:
expected.add('{0}-{1}-py{2[0]}.{2[1]}-colorama{3}-six.json'.format(
commit[:8], tool_name, sys.version_info, psver))

result_files = os.listdir(join(tmpdir, 'results_workflow', 'orangutan'))

assert set(result_files) == expected

# With branches in config
conf.branches = ['master', 'some-branch']
for branches, expected_commits in (
# Without branches in config, shoud just use master
([None], [initial_commit, master_commit]),

_test_run_branches(tmpdir, dvcs, conf, machine_file, 'NEW',
branches=['master', 'some-branch'], initial_commit=initial_commit)
rollback()
# With one branch in config, should just use that branch
(["some-branch"], [initial_commit, branch_commit]),

_test_run_branches(tmpdir, dvcs, conf, machine_file, 'ALL',
branches=['master', 'some-branch'], initial_commit=initial_commit)
# With two branch in config, should apply to specified branches
(["master", "some-branch"], [initial_commit, master_commit, branch_commit]),
):
for range_spec in (None, "NEW", "ALL"):
_test_run(range_spec, branches, expected_commits)

0 comments on commit b7e237c

Please sign in to comment.