[16.0][IMP] hr_timesheet_sheet: split test file to reuse setup #706
Conversation
bealdav
changed the title
hr_timesheet_sheet/tests/__init__.py
Outdated
| @@ -1,3 +1,3 @@ | |||
| # License AGPL-3.0 or later (https://www.gnu.org/licenses/agpl). | |||
|
|
|||
| from . import test_hr_timesheet_sheet | |||
| from . import test_hr_timesheet_sheet_ | |||
There was a problem hiding this comment.
To keep blame as before I use this flow:
cp file new; mv file new2; commit; upd files; commit
Then I need to rename initial file. If not blame is moved to my name
There was a problem hiding this comment.
No, that's not something you can control. Try to blame on your fork and you'll see. One of the 2 parts won't have knowledge of this "split". I also prefer to keep them together. The common.py file is not following the naming convention (using the class name as the file name), and may be a disaster box for a lot of classes.
There was a problem hiding this comment.
test_hr_timesheet_sheet_common?
There was a problem hiding this comment.
@pedrobaeza FYI it can be controlled yes, look here
https://github.com/OCA/timesheet/blame/d692b6832d0fa4f23db2891d9b4ce8f5065ad2be/hr_timesheet_sheet/tests/common.py
https://github.com/OCA/timesheet/blame/d692b6832d0fa4f23db2891d9b4ce8f5065ad2be/hr_timesheet_sheet/tests/test_hr_timesheet_sheet_.py
But i will update only 1 file in future commit
There was a problem hiding this comment.
Good that this works. I think that when more and more the diff diverges, Git/GitHub won't be able to distinguish the split. I have already found it when blaming long history files.
hr_timesheet_sheet/tests/common.py
Outdated
| from odoo.tests.common import TransactionCase | ||
|
|
||
|
|
||
| class HrTimesheetSheetCommon(TransactionCase): |
There was a problem hiding this comment.
Please don't change the file, as this is not friendly for blame operations. Keep it everything in the same file.
There was a problem hiding this comment.
Hi Pedro don't blame me ;- ), I use setup in my PR.
That's the purpose of this PR
Check #704
There was a problem hiding this comment.
Maybe you want I change class to setup but let code in the same file ?
There was a problem hiding this comment.
Yeah, that's what I'm stating: I want to split the test classes, but in the same file, not putting the common.py file.
There was a problem hiding this comment.
Much better in readability. About the guideline of splitting, there are 2 things:
- I don't like to call it
common.py. That's a convention not asked for and not self-descriptive. I know it's somehow inherited from Odoo core, but we mimic from them the things that are OK, and this one isn't personally. Instead, I prefer to still name it with the class name. For this case:test_hr_timesheet_common.py. - I agree about having separate files for classes, but when the splitting is in the middle of the lifecycle of the module, better to keep it in the same file for all the blame and diff related queries.
/ocabot merge patch
|
On my way to merge this fine PR! |
pedrobaeza
changed the title
|
What a great day to merge this nice PR. Let's do it! |
|
Congratulations, your PR was merged at 9ef0511. Thanks a lot for contributing to OCA. ❤️ |
|
Thanks a lot @pedro for this quick merge |
blame is kept after split files
use case #704
cc @pedrobaeza @MiquelRForgeFlow
EDIT :
finally a shorter commit.
Thanks to all for your review and advices