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

[ADD] intrastat_base tests #9

Merged
merged 3 commits into from
Jun 20, 2018
Merged

[ADD] intrastat_base tests #9

merged 3 commits into from
Jun 20, 2018

Conversation

thomaspaulb
Copy link

Added tests.

@coveralls
Copy link

coveralls commented Dec 2, 2016 •
edited
Loading

Coverage Status

Coverage increased (+1.2%) to 47.341% when pulling bee75d3 on sunflowerit:8.0-intrastat_base-tests into b4420bf on OCA:8.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 47.341% when pulling ab60489 on sunflowerit:8.0-intrastat_base-tests into b4420bf on OCA:8.0.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage increased (+1.2%) to 47.341% when pulling ab60489 on sunflowerit:8.0-intrastat_base-tests into b4420bf on OCA:8.0.

@hbrunn hbrunn modified the milestones: 9.0, 8.0 Dec 2, 2016
hbrunn
hbrunn approved these changes Dec 2, 2016
View reviewed changes
Copy link
Member

@hbrunn hbrunn left a comment

Choose a reason for hiding this comment

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

only a small nag

intrastat_base/tests/test_all.py Outdated
"""Tests for this module"""

def setUp(self):
super(TestIntrastatBase, self).setUp()
Copy link
Member

Choose a reason for hiding this comment

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

this is a noop

@pedrobaeza
Copy link
Member

Why tests are not in their respective modules?

@astirpe astirpe mentioned this pull request Jun 12, 2018
Closed
astirpe
astirpe approved these changes Jun 12, 2018
View reviewed changes
Copy link
Member

@astirpe astirpe left a comment

Choose a reason for hiding this comment

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

The setUp() method should be removed. Other than that LGTM.

@thomaspaulb
Copy link
Author

@astirpe @hbrunn Thanks and done.
@pedrobaeza Because the functionality tested is in this module, not in others. It's just to increase coverage.

@pedrobaeza pedrobaeza merged commit 5169f04 into OCA:8.0 Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hbrunn hbrunn hbrunn approved these changes

@astirpe astirpe astirpe approved these changes

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@thomaspaulb @coveralls @pedrobaeza @hbrunn @astirpe