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

Add processing selectors and tests. #75

Conversation

allanleal
Copy link
Contributor

@allanleal allanleal commented Feb 13, 2019

Need your help to check if tests are passing or failing (or if they are adequate).

I'll update the docs shortly.

This implements the features discussed in #74 .

def test_jinja_x86(monkeypatch):
template = "{{ x86 }}"

platform_bkp = platform
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need help here to mock this properly. I need to enforce platform.machine() returns a given value (e.g., 'x86')

Copy link
Member

Choose a reason for hiding this comment

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

You can use monkeypatch.setattr(platform, "machine", lambda: 'x86') and let pytest revert it back for yoy; it is fine to call monkeypatch.setattr multiple times too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm I was almost there, but doing monkeypatch.setattr(platform, "machine", 'x86') instead, which caused problems later.

@nicoddemus
Copy link
Member

Hi @allanleal,

Thanks for the PR!

Need your help to check if tests are passing or failing (or if they are adequate).

Usually we see the CI status here on the PR, but because your last commit in the branch had the string [skip ci], both Travis and AppVeyor did not post the status here. You can see the last time they built in:

https://travis-ci.org/ESSS/conda-devenv/builds/492745664?utm_source=github_status&utm_medium=notification
https://ci.appveyor.com/project/ESSS/conda-devenv/builds/22342149

This implements the features discussed in #74 .

Small GitHub tip: if write Fix #74 or Fixes #74, then #74 will be automatically closed when the PR gets merged to master. ;) More info: https://help.github.com/articles/closing-issues-using-keywords/

conda_devenv/devenv.py Outdated Show resolved Hide resolved
def test_jinja_x86(monkeypatch):
template = "{{ x86 }}"

platform_bkp = platform
Copy link
Member

Choose a reason for hiding this comment

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

You can use monkeypatch.setattr(platform, "machine", lambda: 'x86') and let pytest revert it back for yoy; it is fine to call monkeypatch.setattr multiple times too.

tests/test_jinja.py Outdated Show resolved Hide resolved
@nicoddemus
Copy link
Member

linting is failing due to an unused variable:

conda_devenv/devenv.py:15:5: F841 local variable 'pyversion' is assigned to but never used
ERROR: InvocationError for command '/home/travis/build/ESSS/conda-devenv/.tox/linting/bin/flake8 conda_devenv' (exited with code 1)

@nicoddemus
Copy link
Member

Tests are passing! 🎉

All is left is updating the docs. 🤗

@allanleal
Copy link
Contributor Author

:) I'll update the docs!

@nicoddemus
Copy link
Member

Thanks a lot @allanleal! 🙇

"linux": islinux,
"linux32": islinux and is32bit,
"linux64": islinux and is64bit,
"armv6l": None,
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to let those with None? I see you didn't add them to the docs.
Could you remove them? And maybe create a issue do add them later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll see if I add them still in this PR. Before merging, I'll make a last call for approval just to be sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up deleting those three variables with None and entering a new Issue - #77 .

docs/conf.py Outdated
@@ -97,7 +97,7 @@
#show_authors = False

# The name of the Pygments (syntax highlighting) style to use.
pygments_style = 'sphinx'
pygments_style = 'default'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this for local testing purposes and it remained here. Anyway, I can revert it back, but this one looks better on readthedocs.

Copy link
Member

@prusse-martin prusse-martin Feb 14, 2019

Choose a reason for hiding this comment

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

We try to use the same theme and styles on all open source projects from ESSS.
I will ask to revert this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, changed to sphinx.

@allanleal
Copy link
Contributor Author

allanleal commented Feb 13, 2019

Okay -- all tests passed but one in travis (for py27). The error is a bit strange:

Downloading archive: https://s3.amazonaws.com/travis-python-archives/binaries/ubuntu/14.04/x86_64/python-3.5.tar.bz2
$ curl -sSf -o python-3.5.tar.bz2 ${archive_url}
curl: (56) SSL read: error:00000000:lib(0):func(0):reason(0), errno 104
Unable to download 3.5 archive. The archive may not exist. Please consider a different version.

Forcing a new build with an amend commit.

docs/conf.py Outdated
@@ -97,7 +97,7 @@
#show_authors = False

# The name of the Pygments (syntax highlighting) style to use.
pygments_style = 'sphinx'
pygments_style = 'default'
Copy link
Member

@prusse-martin prusse-martin Feb 14, 2019

Choose a reason for hiding this comment

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

We try to use the same theme and styles on all open source projects from ESSS.
I will ask to revert this.

docs/conf.py Outdated
@@ -111,7 +111,7 @@

# The theme to use for HTML and HTML Help pages. See the documentation for
# a list of builtin themes.
html_theme = 'default'
html_theme = 'sphinx'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops...it's not here, but on pygments_style.

@prusse-martin prusse-martin merged commit 54a7366 into ESSS:master Feb 14, 2019
@allanleal
Copy link
Contributor Author

Great - thanks @prusse-martin .

@prusse-martin
Copy link
Member

Thank you for the work.
I did squash the commits to get get the repo's history clearer =)

@allanleal
Copy link
Contributor Author

allanleal commented Feb 14, 2019

I did squash the commits to get get the repo's history clearer =)

That's OK by me - no worries!

@ESSS ESSS deleted a comment Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants