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

Jump to bottom

Fix some tests #215

Merged
merged 1 commit into from
Oct 24, 2016
Merged

Fix some tests #215

merged 1 commit into from
Oct 24, 2016

Conversation

simahawk
Copy link
Contributor

  1. currently the test test_default_binder is not tested at all on runbot
  2. fix test_default_binder
  3. make test_job work with nose

Unfortunately doctests loader in test_runner_channels.py do not work with nose, it throws this error:

ERROR: connector.tests.test_runner_channels.load_tests
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/path/to/nose-1.3.7-py2.7.egg/nose/case.py", line 197, in runTest
    self.test(*self.arg)
TypeError: load_tests() takes exactly 3 arguments (0 given)

simahawk
simahawk commented Oct 12, 2016
View reviewed changes
connector/connector.py
@@ -443,10 +443,11 @@ def unwrap_binding(self, binding_id, browse=False):
else:
binding = self.model.browse(binding_id)

openerp_record = getattr(binding, self._openerp_field)
# XXX: do we expect to have a m2o field here?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@guewen this is not clear to me

Copy link
Member

Choose a reason for hiding this comment

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

self._openerp_field should be the m2o fields towards the "normal" record (the m2o used in the _inherits), does it answer to you question?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed the test as we said ;)

@guewen guewen mentioned this pull request Oct 12, 2016
Merged
@lasley lasley added this to the 9.0 milestone Oct 14, 2016
lasley
lasley reviewed Oct 18, 2016
View reviewed changes
connector/connector.py
@@ -443,10 +443,10 @@ def unwrap_binding(self, binding_id, browse=False):
else:
binding = self.model.browse(binding_id)

openerp_record = getattr(binding, self._openerp_field)
record = getattr(binding, self._openerp_field)
Copy link
Contributor

@lasley lasley Oct 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

This is a security issue. Does binding[self._openerp_field] work?

Copy link
Contributor

Choose a reason for hiding this comment

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

@lasley IMO it's not a security issue since _openerp_field is defined in the source code and not exposed to the ui...

Copy link
Contributor

Choose a reason for hiding this comment

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

@lmignon - Seems like a best practice to use item lookup instead of attribute for recordsets IMO, but you are correct that it's not editable in the UI. I'll remove the PR block

Copy link
Contributor Author

@simahawk simahawk Oct 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@lasley @lmignon actually I planned to go trough all these getattr (there are several of them) and replace them, but as Laurent pointed out there's no user input so I think that we can live with this for a while :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for the record #218 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@lasley You are right, it's a best practice 😏

lasley
lasley suggested changes Oct 18, 2016
View reviewed changes
Copy link
Contributor

@lasley lasley left a comment

Choose a reason for hiding this comment

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

One note + please rebase & resolve conflict

@lmignon
Copy link
Contributor

lmignon commented Oct 19, 2016

@simahawk Can you rebase and resolve the conflict?

@simahawk
Copy link
Contributor Author

@lmignon should be done :)

@lasley lasley mentioned this pull request Oct 19, 2016
Closed
@lasley lasley merged commit ef4ed1c into OCA:9.0 Oct 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guewen guewen guewen left review comments

@lasley lasley lasley approved these changes

@lmignon lmignon lmignon approved these changes

Assignees
No one assigned
Labels
needs review
Projects
None yet
Milestone
9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@simahawk @lmignon @guewen @lasley