From b7e237c27cef0c5dd1923e30c40a8d41ccdfa505 Mon Sep 17 00:00:00 2001 From: Philippe Pepiot Date: Wed, 8 Jun 2016 15:15:00 +0200 Subject: [PATCH] Always use configured branches in run, profiling and continuous commands 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 --- asv/commands/continuous.py | 8 ++-- asv/commands/profiling.py | 5 +-- asv/commands/run.py | 4 +- asv/environment.py | 4 +- asv/plugins/git.py | 2 + asv/plugins/mercurial.py | 2 + asv/repo.py | 6 --- test/test_workflow.py | 83 ++++++++++++++++---------------------- 8 files changed, 48 insertions(+), 66 deletions(-) diff --git a/asv/commands/continuous.py b/asv/commands/continuous.py index e60f435fe..72a5dd2a6 100644 --- a/asv/commands/continuous.py +++ b/asv/commands/continuous.py @@ -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) @@ -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: diff --git a/asv/commands/profiling.py b/asv/commands/profiling.py index 50c97faca..7f26a1a76 100644 --- a/asv/commands/profiling.py +++ b/asv/commands/profiling.py @@ -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() diff --git a/asv/commands/run.py b/asv/commands/run.py index 4a3e17ab5..c80ba2779 100644 --- a/asv/commands/run.py +++ b/asv/commands/run.py @@ -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 @@ -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": diff --git a/asv/environment.py b/asv/environment.py index 646178de0..84dabc0ae 100644 --- a/asv/environment.py +++ b/asv/environment.py @@ -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) diff --git a/asv/plugins/git.py b/asv/plugins/git.py index ba20bc811..0e676efa5 100644 --- a/asv/plugins/git.py +++ b/asv/plugins/git.py @@ -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] diff --git a/asv/plugins/mercurial.py b/asv/plugins/mercurial.py index 7f9451ce3..06340ed52 100644 --- a/asv/plugins/mercurial.py +++ b/asv/plugins/mercurial.py @@ -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): diff --git a/asv/repo.py b/asv/repo.py index 9ca69e17b..f49963ba7 100644 --- a/asv/repo.py +++ b/asv/repo.py @@ -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. diff --git a/test/test_workflow.py b/test/test_workflow.py index 4cdd18eca..0ec021261 100644 --- a/test/test_workflow.py +++ b/test/test_workflow.py @@ -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 @@ -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+"^!", @@ -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)