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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix RQD_TAGS #916

Merged
merged 5 commits into from
Feb 10, 2021
Merged

Fix RQD_TAGS #916

merged 5 commits into from
Feb 10, 2021

Conversation

splhack
Copy link
Contributor

@splhack splhack commented Feb 9, 2021 •
edited

  • RQD_TAGS didn't work due to the method (getint vs get) in the first place 馃槄

  • Added unit tests with mocking rqd.conf

@splhack
Copy link
Contributor Author

splhack commented Feb 9, 2021 •
edited

will update DEFAULT_FACILITY code/test when #911 landed.
Merged #911 partially

@splhack
Copy link
Contributor Author

splhack commented Feb 9, 2021

Skipped the unittests on CY2019 CI.
I can't repro it error locally with macOS + Python 2.7.16 though.

$ pytest rqconstants_tests.py
======================================= test session starts ========================================
platform darwin -- Python 2.7.16, pytest-4.6.11, py-1.10.0, pluggy-0.13.1
plugins: pyfakefs-3.6
collected 2 items

rqconstants_tests.py ..                                                                      [100%]

===================================== 2 passed in 0.63 seconds =====================================

bcipriano
bcipriano reviewed Feb 9, 2021
View reviewed changes
Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

Thanks!

Change looks good, but I've merged #911 now so could you merge master on your branch to make sure we have a clean diff?

@splhack splhack changed the title Fix rqconstants Fix RQD_TAGS Feb 9, 2021
@splhack
Copy link
Contributor Author

splhack commented Feb 9, 2021

@bcipriano sure, I've just rebased.

@bcipriano
Copy link
Collaborator

Thanks, change LGTM.

What was the test failure under Python 2? I don't see it listed here. Maybe I can point you in the right direction -- would love to avoid disabling it if we can.

@splhack
Copy link
Contributor Author

splhack commented Feb 9, 2021

CY2019 (Python 2.7.15) looks like failed to read config file,
whereas my local Python 2.7.16 (macOS 10.15 default /usr/bin/python2) works as expected.

$ pytest -v rqconstants_tests.py
======================================= test session starts ========================================
platform darwin -- Python 2.7.16, pytest-4.6.11, py-1.10.0, pluggy-0.13.1 -- venv2/bin/python
cachedir: .pytest_cache
rootdir: OpenCue/rqd
plugins: pyfakefs-3.6
collected 2 items

rqconstants_tests.py::RqConstantTests::test_facility PASSED                                  [ 50%]
rqconstants_tests.py::RqConstantTests::test_tags PASSED                                      [100%]

===================================== 2 passed in 0.38 seconds =====================================

@splhack
Copy link
Contributor Author

splhack commented Feb 9, 2021 •
edited

oh, ok, so, the cause is CY2019 doesn't have configparser.

test_facility (tests.rqconstants_tests.RqConstantTests) ... WARNING:root:Failed to read values from config file /tmp/tmpA8NU0D/ed095071-9739-41c5-b235-6899425ea247 due to No module named configparser at [('/__w/OpenCue/OpenCue/rqd/rqd/rqconstants.py', 131, '', None)]

@splhack
Copy link
Contributor Author

splhack commented Feb 9, 2021

progress. test_facility passed with installing configparser.

But test_tags failed. something different between pytest vs python rqd/setup.py test.

@splhack
Copy link
Contributor Author

splhack commented Feb 9, 2021

ok, added the same treatment as PyOutline

OpenCue/pyoutline/outline/config.py

Lines 35 to 41 in 70e4776

import six
from six.moves import configparser
if six.PY2:
ConfigParser = configparser.SafeConfigParser
else:
ConfigParser = configparser.ConfigParser

bcipriano
bcipriano reviewed Feb 9, 2021
View reviewed changes
Copy link
Collaborator

@bcipriano bcipriano left a comment

Choose a reason for hiding this comment

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

Nice! Just one last question.

requirements.txt Outdated
@@ -1,4 +1,5 @@
2to3==1.0
configparser==4.0.2;python_version<"3.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

If configparser is coming from the six library now, do we still need this new dependency?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess not. deleted.

@bcipriano bcipriano merged commit 2ff7e23 into AcademySoftwareFoundation:master Feb 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bcipriano bcipriano bcipriano approved these changes

@DiegoTavares DiegoTavares Awaiting requested review from DiegoTavares DiegoTavares is a code owner

@gregdenton gregdenton Awaiting requested review from gregdenton

@IdrisMiles IdrisMiles Awaiting requested review from IdrisMiles

@jrray jrray Awaiting requested review from jrray

@larsbijl larsbijl Awaiting requested review from larsbijl

@smith1511 smith1511 Awaiting requested review from smith1511

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@splhack @bcipriano