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.

Sign up for GitHub

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

Move fixtures from test/test_dev.py file #1124

Closed
wants to merge 4 commits into from
Closed

Move fixtures from test/test_dev.py file #1124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
94d6037
Fix fixtures
dorothykiz1 Mar 9, 2022
a0aec74
sort imports
dorothykiz1 Mar 9, 2022
2a3d906
sort imports
dorothykiz1 Mar 9, 2022
b1c9ffb
fix ValueError: too many values to unpack
dorothykiz1 Mar 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions test/test_check.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,10 @@
from asv import util

from . import tools
from .test_dev import basic_conf


def test_check(capsys, basic_conf):
tmpdir, local, conf = basic_conf
tmpdir, local, conf, _ = basic_conf
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning here? Why do you think this is the best solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could have used * operator to pack the remaining elements into a list but since it was a single element remaining to be returned i decided to use _ the dummy variable to unpack it (as what was expected was 3 and the values returned on the left were 4 , the remaining element had no variable, i assigned a dummy variable _)

Copy link
Member

Choose a reason for hiding this comment

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

Did you check why the function returns 4, and 3 are expected? My question is not about why underscore or star. The question is why if this used to return 3, why it now returns 4, and receiving one more parameter is your preferred solution (as opposed to make the function to keep returning 3, as it was before)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes,An extra variable machine_file was added to be returned that stores the hostname machine details. This makes the variables to be returned 4, and yet when the function was being called in different files earlier it was unpacking 3 variables, hence the need to add the dummy variable to match what the function is returning.

Copy link
Member

Choose a reason for hiding this comment

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

I see. But this don't make sense. The reason why a different number of variables are being returned is because with your changes, a different function is being called. So different things are happening in the tests in your branch, and in master. Which is obviously something we don't want, unless you've got a reason to change the behavior, which doesn't seem it is the case.


# Test check runs (with full benchmark suite)
with pytest.raises(util.UserError, match="Benchmark suite check failed"):
Expand Down
22 changes: 5 additions & 17 deletions test/test_dev.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
import shutil
from os.path import abspath, dirname, join, relpath

import pytest

from asv import config
from asv.commands import make_argparser

Expand Down Expand Up @@ -50,18 +48,8 @@ def generate_basic_conf(tmpdir, repo_subdir=''):
return tmpdir, local, conf


@pytest.fixture
def basic_conf(tmpdir):
return generate_basic_conf(tmpdir)


@pytest.fixture
def basic_conf_with_subdir(tmpdir):
return generate_basic_conf(tmpdir, 'some_subdir')


def test_dev(capsys, basic_conf):
tmpdir, local, conf = basic_conf
tmpdir, local, conf, _ = basic_conf

# Test Dev runs (with full benchmark suite)
ret = tools.run_asv_with_conf(conf, 'dev', '--quick', '-e',
Expand All @@ -82,7 +70,7 @@ def test_dev_with_repo_subdir(capsys, basic_conf_with_subdir):
"""
Same as test_dev, but with the Python project inside a subdirectory.
"""
tmpdir, local, conf = basic_conf_with_subdir
tmpdir, local, conf, _ = basic_conf_with_subdir

# Test Dev runs
tools.run_asv_with_conf(conf, 'dev', '--quick',
Expand All @@ -99,15 +87,15 @@ def test_dev_with_repo_subdir(capsys, basic_conf_with_subdir):


def test_dev_strict(basic_conf):
tmpdir, local, conf = basic_conf
tmpdir, local, conf, _ = basic_conf
ret = tools.run_asv_with_conf(conf, 'dev', '--strict', '--quick',
'--bench=TimeSecondary',
_machine_file=join(tmpdir, 'asv-machine.json'))
assert ret == 2


def test_run_python_same(capsys, basic_conf):
tmpdir, local, conf = basic_conf
tmpdir, local, conf, _ = basic_conf

# Test Run runs with python=same
tools.run_asv_with_conf(conf, 'run', '--python=same',
Expand All @@ -125,7 +113,7 @@ def test_run_python_same(capsys, basic_conf):


def test_profile_python_same(capsys, basic_conf):
tmpdir, local, conf = basic_conf
tmpdir, local, conf, _ = basic_conf

# Test Profile can run with python=same
tools.run_asv_with_conf(conf, 'profile', '--python=same', "time_secondary.track_value",
Expand Down