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 cirq.testing.assert_same_diagram. #917

Merged
merged 6 commits into from
Sep 21, 2018

Conversation

dlyongemallo
Copy link
Contributor

Fixes #830.

@googlebot googlebot added the cla: yes Makes googlebot stop complaining. label Sep 14, 2018
@dlyongemallo dlyongemallo force-pushed the assert_same_diagram branch 2 times, most recently from 4320668 to cf3f800 Compare September 17, 2018 09:48
Copy link
Contributor

@Strilanc Strilanc left a comment

Choose a reason for hiding this comment

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

Thanks, this is exactly what I had in mind! Just a few comments.

@Strilanc
Copy link
Contributor

It looks like zip_longest isn't present in python 2. You'll have to use something else.

@dlyongemallo
Copy link
Contributor Author

Python 2 has izip_longest. Is there a way to specify this should be used during the transpilation?

@Strilanc
Copy link
Contributor

We could search replace itertools.zip_longest with itertools.izip_longest in the conversion script.

@dlyongemallo
Copy link
Contributor Author

E UnicodeEncodeError: 'ascii' codec can't encode characters in position 88-90: ordinal not in range(128)

Hmm, it seems I can't use and am restricted to range(128) in Python 2 if I return str.

@dlyongemallo dlyongemallo force-pushed the assert_same_diagram branch 2 times, most recently from 7d39868 to 2feecd4 Compare September 19, 2018 09:28
@Strilanc
Copy link
Contributor

Strilanc commented Sep 19, 2018

Where is that being triggered? In general, we try to make sure unicode works even in python 2.

@dlyongemallo
Copy link
Contributor Author

In test_assert_has_diagram() when running continuous-integration/check.sh --only=pytest2. Actually, it isn't just the one character , it seems that the entire string (the parts with non-ascii, e.g., the wires) is a problem.

(btw is there a way to specify running a specific test when running the python2 virtual environment using check.sh --only=python2? I mean like with the -k parameter of pytest.)

@dlyongemallo
Copy link
Contributor Author

dlyongemallo commented Sep 20, 2018

Strings don't automatically handle unicode in python2 so we'd have to change the test for python2 (and change _text_diagram_diff to return unicode) if we want it to run.

I've marked the test @cirq.testing.only_test_in_python3 for now.

@dlyongemallo
Copy link
Contributor Author

So one way I can fix this is to wrap the _text_diagram_diff in a private class with a __str__ method, and ditto for the assignment of expected_error in circuit_compare_test.py, so that the python2.7-generate.sh would take care of the unicode encoding. That seems kludgy though.

@Strilanc
Copy link
Contributor

It doesn't make sense that this specific test would have a failure when all the other ones work fine. We do quite a lot of tests involving unicode strings! There is something unique about this one that I don't understand that is triggering the issue.

@Strilanc
Copy link
Contributor

Strilanc commented Sep 20, 2018

It's something to do with pytest.match specifically

@Strilanc
Copy link
Contributor

Here's my current workaround:

    with pytest.raises(AssertionError) as ex_info:
        cirq.testing.assert_has_diagram(circuit, u"""
0: ───@───

1: ───Z───
""")
    assert expected_error in ex_info.value.args[0]

@Strilanc
Copy link
Contributor

This is a very strangely behaved thing. This passes:

    with pytest.raises(AssertionError, match=u'█'.encode('utf-8')):
        raise AssertionError(u'█'.encode('utf-8'))

Whereas this fails:

    with pytest.raises(AssertionError, match=u'█'.encode('utf-8')):
        assert False, u'█'.encode('utf-8')

@dlyongemallo
Copy link
Contributor Author

Could be a bug in pytest. It looks similar to this one: pytest-dev/pytest#877

@dlyongemallo
Copy link
Contributor Author

I've added a commit with your work around.

(btw is there a way to do a diffbase in github, i.e., work on top of an uncommitted PR? This particular change is super useful for doing other stuff, e.g., #918.)

@Strilanc
Copy link
Contributor

The closest thing is to create a PR that merges into the other branch. Unfortunately, this means that none of the branch protections kick in (e.g. requiring a review, running status checks).

@Strilanc Strilanc added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 21, 2018
@CirqBot
Copy link
Collaborator

CirqBot commented Sep 21, 2018

Automerge: started

@CirqBot CirqBot merged commit c1e8e5f into quantumlib:master Sep 21, 2018
@CirqBot
Copy link
Collaborator

CirqBot commented Sep 21, 2018

Automerge done.

@CirqBot CirqBot removed the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Sep 21, 2018
@dlyongemallo dlyongemallo deleted the assert_same_diagram branch September 21, 2018 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants