Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1945733002:
[Battor] Fix broken BattOr unit tests and set them to run in CQ. (Closed)
|
Created:
4 years, 7 months ago by rnephew (Reviews Here) Modified:
4 years, 7 months ago CC:
catapult-reviews_chromium.org Base URL:
git@github.com:catapult-project/catapult@master Target Ref:
refs/heads/master Project:
catapult Visibility:
Public. |
Description[Battor] Fix broken BattOr unit tests and set them to run in CQ.
BUG=catapult:#2202
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/239f8ce9af4746fd52f2f0ab6c3fbcb740eb400d
Patch Set 1 #
Total comments: 4
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #Patch Set 4 : fix whitespace #
Messages
Total messages: 22 (7 generated)
rnephew@chromium.org changed reviewers: + charliea@chromium.org, nednguyen@google.com, sullivan@chromium.org
https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... common/battor/battor/battor_wrapper_unittest.py:10: os.path.join(os.path.dirname(__file__), '..')) The general guidance in catapult is to keep sys.path manipulation in the __init__.py file: https://github.com/catapult-project/catapult/blob/master/docs/directory-struc... Is there a reason that can't be done here?
https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... common/battor/battor/battor_wrapper_unittest.py:10: os.path.join(os.path.dirname(__file__), '..')) On 2016/05/03 16:51:52, sullivan wrote: > The general guidance in catapult is to keep sys.path manipulation in the > __init__.py file: > https://github.com/catapult-project/catapult/blob/master/docs/directory-struc... > > Is there a reason that can't be done here? Doing it there yields: Traceback (most recent call last): File "battor_wrapper_unittest.py", line 13, in <module> from battor import battor_wrapper ImportError: No module named battor I wish I knew how to make it pick up the __init__, but I dont and searching for how has not really come back with any useful answer.
https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:5: import __init__ Does this work better for catapult style?
aiolos@chromium.org changed reviewers: + aiolos@chromium.org
https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... common/battor/battor/battor_wrapper_unittest.py:10: os.path.join(os.path.dirname(__file__), '..')) On 2016/05/03 16:59:11, rnephew1 wrote: > On 2016/05/03 16:51:52, sullivan wrote: > > The general guidance in catapult is to keep sys.path manipulation in the > > __init__.py file: > > > https://github.com/catapult-project/catapult/blob/master/docs/directory-struc... > > > > Is there a reason that can't be done here? > > Doing it there yields: > > Traceback (most recent call last): > File "battor_wrapper_unittest.py", line 13, in <module> > from battor import battor_wrapper > ImportError: No module named battor > > I wish I knew how to make it pick up the __init__, but I dont and searching for > how has not really come back with any useful answer. Generally it will give that kind of error if you have a circular dependency while importing the files in the module. https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:5: import __init__ On 2016/05/03 17:16:16, rnephew1 wrote: > Does this work better for catapult style? Instead of doing this, I would suggest adding the file(s) that are part of the API to the init file. You should then just be able to do a straight 'from battor import battor_wrapper'. The error you were seeing earlier often happens if there is a (possible) circular dependency while importing things within the module. Possible work arounds: 1) You may need to use relative imports in the init file. See here for an example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... 2) Or you may need to play with the import order a bit to avoid circular dependencies. See here for an example: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... (page imports story, so the init file imports story before page.)
https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... common/battor/battor/battor_wrapper_unittest.py:10: os.path.join(os.path.dirname(__file__), '..')) On 2016/05/03 19:17:25, aiolos(slow reviews) wrote: > On 2016/05/03 16:59:11, rnephew1 wrote: > > On 2016/05/03 16:51:52, sullivan wrote: > > > The general guidance in catapult is to keep sys.path manipulation in the > > > __init__.py file: > > > > > > https://github.com/catapult-project/catapult/blob/master/docs/directory-struc... > > > > > > Is there a reason that can't be done here? > > > > Doing it there yields: > > > > Traceback (most recent call last): > > File "battor_wrapper_unittest.py", line 13, in <module> > > from battor import battor_wrapper > > ImportError: No module named battor > > > > I wish I knew how to make it pick up the __init__, but I dont and searching > for > > how has not really come back with any useful answer. > > Generally it will give that kind of error if you have a circular dependency > while importing the files in the module. Slightly different reason this time. This is being run as a script, not being imported module; which is why the init isn't being called. __init__ is only called when importing a module. Finally found something about it. https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... File common/battor/battor/battor_wrapper_unittest.py (right): https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... common/battor/battor/battor_wrapper_unittest.py:5: import __init__ On 2016/05/03 19:17:25, aiolos(slow reviews) wrote: > On 2016/05/03 17:16:16, rnephew1 wrote: > > Does this work better for catapult style? > > Instead of doing this, I would suggest adding the file(s) that are part of the > API to the init file. You should then just be able to do a straight 'from battor > import battor_wrapper'. The error you were seeing earlier often happens if there > is a (possible) circular dependency while importing things within the module. > > Possible work arounds: > 1) You may need to use relative imports in the init file. See here for an > example: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... > > 2) Or you may need to play with the import order a bit to avoid circular > dependencies. See here for an example: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... > > (page imports story, so the init file imports story before page.) See other comment on why __init__ is not being run.
On 2016/05/03 19:22:34, rnephew1 wrote: > https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... > File common/battor/battor/battor_wrapper_unittest.py (right): > > https://codereview.chromium.org/1945733002/diff/1/common/battor/battor/battor... > common/battor/battor/battor_wrapper_unittest.py:10: > os.path.join(os.path.dirname(__file__), '..')) > On 2016/05/03 19:17:25, aiolos(slow reviews) wrote: > > On 2016/05/03 16:59:11, rnephew1 wrote: > > > On 2016/05/03 16:51:52, sullivan wrote: > > > > The general guidance in catapult is to keep sys.path manipulation in the > > > > __init__.py file: > > > > > > > > > > https://github.com/catapult-project/catapult/blob/master/docs/directory-struc... > > > > > > > > Is there a reason that can't be done here? > > > > > > Doing it there yields: > > > > > > Traceback (most recent call last): > > > File "battor_wrapper_unittest.py", line 13, in <module> > > > from battor import battor_wrapper > > > ImportError: No module named battor > > > > > > I wish I knew how to make it pick up the __init__, but I dont and searching > > for > > > how has not really come back with any useful answer. > > > > Generally it will give that kind of error if you have a circular dependency > > while importing the files in the module. > > Slightly different reason this time. This is being run as a script, not being > imported module; which is why the init isn't being called. __init__ is only > called when importing a module. Finally found something about it. > > https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... > File common/battor/battor/battor_wrapper_unittest.py (right): > > https://codereview.chromium.org/1945733002/diff/20001/common/battor/battor/ba... > common/battor/battor/battor_wrapper_unittest.py:5: import __init__ > On 2016/05/03 19:17:25, aiolos(slow reviews) wrote: > > On 2016/05/03 17:16:16, rnephew1 wrote: > > > Does this work better for catapult style? > > > > Instead of doing this, I would suggest adding the file(s) that are part of the > > API to the init file. You should then just be able to do a straight 'from > battor > > import battor_wrapper'. The error you were seeing earlier often happens if > there > > is a (possible) circular dependency while importing things within the module. > > > > Possible work arounds: > > 1) You may need to use relative imports in the init file. See here for an > > example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... > > > > 2) Or you may need to play with the import order a bit to avoid circular > > dependencies. See here for an example: > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/catapu... > > > > (page imports story, so the init file imports story before page.) > > See other comment on why __init__ is not being run. You should make common/battor/bin/run_tests that run the unittests instead
> You should make common/battor/bin/run_tests that run the unittests instead +1.
On 2016/05/03 20:08:57, aiolos(slow reviews) wrote: > > You should make common/battor/bin/run_tests that run the unittests instead > > +1. Done.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by nednguyen@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1945733002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945733002/80001
lgtm
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rnephew@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1945733002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1945733002/80001
Message was sent while issue was closed.
Description was changed from ========== [Battor] Fix broken BattOr unit tests and set them to run in CQ. BUG=catapult:#2202 ========== to ========== [Battor] Fix broken BattOr unit tests and set them to run in CQ. BUG=catapult:#2202 Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapu... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001) as https://chromium.googlesource.com/external/github.com/catapult-project/catapu... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
