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

Fix mechRepr on controls to always return a native str. #36

Merged
merged 1 commit into from Oct 18, 2017

Conversation

icemac
Copy link
Member

@icemac icemac commented Oct 16, 2017

The error scenario is the following:

  • A page contains a submit control whose value contains a non-ASCII character.
  • There is also a select with options or a text input field.
  • browser.getControl() is used to select a not existing control.
  • TestBrowser tries to render the list of possible controls using the mechRepr of the controls.
  • The previous step fails with a UnicodeDecodeError when joining the mechRepr values of the controls because the ones of select option (aka item) and text field are unicode while the one of the submit control is str but contains a non-ASCII char.

As the scenario described above requires the mechRepr to be of the same type I decided to adapt the two remaining control classes which returned unicode to the str approach used by of the majority of the controls.

Fixes #38.

@icemac icemac requested a review from mgedmin October 16, 2017 17:09
@icemac
Copy link
Member Author

icemac commented Oct 16, 2017

@mgedmin I am not sure why GitHub thinks that you are the author of this commit. Maybe my git-foo is only too little.

@mgedmin
Copy link
Member

mgedmin commented Oct 17, 2017

@icemac have you perchance copied my ~/.gitconfig?

Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

I'm not very happy to see a commit I didn't write attributed to me. You should be able to fix that with git commit --amend --author="Full Name <email@example.com>", followed by git push --force-with-lease.

Otherwise LGTM!

CHANGES.rst Outdated
@@ -5,7 +5,7 @@ CHANGES
5.2.3 (unreleased)
------------------

- Nothing changed yet.
- Fix ``mechRepr`` on controls to always return a native str.
Copy link
Member

Choose a reason for hiding this comment

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

Link to GitHub issue/pull request for more context?

suite = unittest.TestSuite()
suite.addTests([
unittest.makeSuite(TestDisplayValue),
unittest.makeSuite(TestMechRepr),
Copy link
Member

Choose a reason for hiding this comment

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

Instead of repeating unittest.makeSuite() for every test class, I recommend unittest.defaultTestLoader.loadTestsFromName(__name__), which finds all test classes automatically.

Fixes #38

As the scenario described in the ticket requires the `mechRepr` to be of the same type for all controls I decided to adapt the two remaining control classes which returned unicode to the str approach used by of the majority of the controls.
@icemac
Copy link
Member Author

icemac commented Oct 17, 2017

@mgedmin Thank you for your review. I fixed the commit as you suggested including the wrong author. (I am not re-using your config, maybe this was an error in my git client.)

Copy link
Member

@mgedmin mgedmin left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

None yet

2 participants