Fix st2 login and st2 whoami commands to work #3211

Kami · 2017-02-10T13:02:44Z

Randomly noticed it's not working while testing it - https://gist.github.com/Kami/f03360208e2e8599f42d3152df054842

It looks like the problem is that code tries to work with the ConfigParser object as a dictionary, but ConfigParser object doesn't behave and support dictionary-like access notation.

Edit: Looked at the tests and it looks like they didn't catch this issue because the hide it because they mock ConfigParser instance. I know that's not always possible, but we should try to avoid mocking as much as possible because many times we end up mocking too much and we actually end up testing mocks and not the actual code...

TODO

Tests (and figure out why existing tests passed, maybe they mock too much. Imo, better and safer approach would be to use actual config file fixtures from disk - this way we don't need to mock stuff.)

Kami · 2017-02-10T13:04:29Z

@Mierdin I'm a bit confused about this - it seems like it should have never worked, or am I missing something?

Kami · 2017-02-10T13:06:45Z

Edit: So it seems like the problem is that only ConfigParser instance in Python 3 works like a dict (https://docs.python.org/3/library/configparser.html#module-configparser), but not one available in Python 2 (https://docs.python.org/2/library/configparser.html).

Did you happen to test it with Python 3 locally (I used Python 2.7 which is the version we officially support)?

We don't fully support Python 3 yet and if anything, we should use the approach which works on both versions where possible :)

Mierdin · 2017-02-10T19:27:23Z

Hrm, this is all very strange. FWIW I was not using Python 3 - my dev box runs 2.7.6.

I'll dig into this today and see if I can't get it straightened out. Apologies if I'm over-relying on mock - I was always told to use mock to avoid testing things I wasn't trying to test. In this case, I don't want to test ConfigParser, I want to test the code that's using it. However, I can see your perspective and will try to find some middle ground.

Kami · 2017-02-10T19:33:35Z

Apologies if I'm over-relying on mock - I was always told to use mock to avoid testing things I wasn't trying to test. In this case, I don't want to test ConfigParser, I want to test the code that's using it. However, I can see your perspective and will try to find some middle ground.

No problem, sometimes it's hard to find the balance what to mock and what to not in the unit tests.

Hrm, this is all very strange. FWIW I was not using Python 3 - my dev box runs 2.7.6.

It looks like this could also happen if you have configparser package installed locally - https://pypi.python.org/pypi/configparser

Afaik, none of our packages and components rely and use that package, but having this package installed locally would cause this behavior.

Mierdin · 2017-02-10T19:49:06Z

@Kami figured it out. This is the culprit - #3194

I was 99% sure I sufficiently tested everything manually after that change, but the only possible explanation is, no I did not. :-/

Anyways - this one's on me - I'll fix it.

The previous version of these tests were a bit too reliant on mock, so this approach instead works with as few mocks as possible. Naturally, we have to keep the mock for the st2 API interactions, but these tests run the `st2 login` and `st2 whoami` commands using real config files, as opposed to a mocked ConfigParser. Signed-off-by: Matt Oswalt <oswaltm@brocade.com>

codecov-io · 2017-02-12T04:02:00Z

Codecov Report

Merging #3211 into master will decrease coverage by -0.17%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #3211      +/-   ##
==========================================
- Coverage    77.8%   77.63%   -0.17%     
==========================================
  Files         433      433              
  Lines       22415    22414       -1     
==========================================
- Hits        17439    17400      -39     
- Misses       4976     5014      +38

Impacted Files	Coverage Δ
st2debug/st2debug/processors.py	`94.29% <100%> (ø)`	✅
st2client/st2client/commands/auth.py	`69.61% <100%> (+1.48%)`	✅
st2client/st2client/utils/logging.py	`32% <ø> (-48%)`	❌
st2client/st2client/base.py	`62.63% <ø> (-9.6%)`	❌
...on/st2common/transport/connection_retry_wrapper.py	`75% <ø> (-5.77%)`	❌
st2common/st2common/query/base.py	`83.46% <ø> (-5.26%)`	❌
st2client/st2client/shell.py	`85.37% <ø> (-1.63%)`	❌
st2common/st2common/services/triggerwatcher.py	`90.14% <ø> (-1.41%)`	❌
st2common/st2common/bootstrap/triggersregistrar.py	`83.33% <ø> (-1.19%)`	❌
st2api/st2api/controllers/v1/triggers.py	`70.94% <ø> (-0.99%)`	❌
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be6acb4...072bd3d. Read the comment docs.

Mierdin · 2017-02-12T04:10:46Z

So....after looking into this a bit more, here's my working theory. It's true that the built-in ConfigParser doesn't allow dict-like references in until Python 3. However, it appears that you can install this package and get this functionality when running Python 2.7.

Looks like this does get installed in the virtualenv when running make requirements, as it is a dependency of flake8:

~$ pipdeptree
....
flake8==3.0.4
  - configparser [required: Any, installed: 3.5.0]
  - enum34 [required: Any, installed: 1.1.6]
  - mccabe [required: >=0.5.0,<0.6.0, installed: 0.5.3]
  - pycodestyle [required: >=2.0.0,<2.1.0, installed: 2.0.0]
  - pyflakes [required: <1.3.0,!=1.2.1,!=1.2.2,>=0.8.1,!=1.2.0, installed: 1.2.3]
....

So, this explains why it was working before the move to using the ConfigParser in six. Before that, the commands worked just fine, because they were using this third-party library that acted like the built-in Python3 ConfigParser.

Now, regarding the errors that came up once #3194 was merged, I'm guessing this is because six's ConfigParser is designed to act like the built-in version for Python 2, which doesn't have __getitem__ access either.

All that said, the changes you made to auth.py are definitely better, and my mocks were clearly wrong as they were built to mimic the wrong ConfigParser version. Once we started using the right version in #3194, these tests were invalid.

I re-did all of the unit tests for this in ea4c121 and onward; hopefully they'll help protect from this a bit better.

Kami · 2017-02-13T08:15:51Z

Yes, I also agree with your assessment :)

And yeah, it's a bit nasty that dev dependencies can interfere with things in suck a sneaky manner...

Kami · 2017-02-13T08:19:24Z

st2client/tests/unit/test_auth.py

+
+class TestLoginUncaughtException(TestLoginBase):
+
+    CONFIG_FILE = '/tmp/logintest.cfg'


Might be a bit safer to use a random temporary path (e.g. mkstemp), besides that, LGTM.

Kami · 2017-02-13T08:21:22Z

It appears e2e tests failed because this branch doesn't contain packs.download fix / changes (the tests are passing on master).

I've merged latest master into this branch, this should fix it.

…o fix_st2_login_command

Kami · 2017-02-13T08:39:07Z

st2client/tests/unit/test_auth.py

 import requests
 import argparse
 import logging
+from cStringIO import StringIO


Minor style / import ordering thing - we tend to put stdlib imports at the top then followed by 3rd party package imports then followed by local components imports.

I follow the same standard, and I know better. Need to figure out why my editor didn't catch this

Kami · 2017-02-13T08:39:58Z

st2client/tests/unit/test_auth.py

 import requests
 import argparse
 import logging
+from cStringIO import StringIO


For cross version compatibility it would be better to use six.StringIO (yeah, this one shouldn't cause issues :D)

Kami · 2017-02-13T08:40:31Z

st2client/tests/unit/test_auth.py

+
+    DOTST2_PATH = os.path.expanduser('~/.st2/')

    def __init__(self, *args, **kwargs):


Instead of using kwargs, it would be a bit more readable to just directly declare arguments in the constructor signature.

class variable.

Kami · 2017-02-13T09:08:09Z

I pushed some changes (41090ab, c1923f6, ba85739, 339af89, 2602577).

I think we might also be able to re-use stdout capturing from base class (no need to create and use CaptureStdout), but I would need to dig in to confirm and something we can simplify in the future (if possible).

Kami · 2017-02-13T09:34:53Z

@Mierdin when you get a chance can you please have a look and approve it (I forgot I opened it and the PR hasn't been approved yet so I can't merge it :))?

otherwise it will be re-used when multiple class inherit from this base class which is undesired since we always want to start with a clean state.

Kami · 2017-02-13T12:50:38Z

It looks like I was also able to replace CaptureStdout with existing stdout capture mechanism in the base test class - 072bd3d. It just required a small fix to the base test class otherwise it wouldn't work when multiple classes inherited from the same bass class - 380b958 (internal state was not reset for each test run).

Kami · 2017-02-13T14:45:14Z

Some of the e2e tests failure appear to be related to intermediate upstream / networking issues.

Mierdin

Nice, thanks for the improvements! LGTM

Mierdin · 2017-02-13T18:33:13Z

@Kami I re-ran the e2e test and it passed. Not sure why Codecov is throwing a fit though.

Fix st2 login command - ConfigParser object doesn't behave like a dict.

929de38

Also fix whoami command.

c8e08f6

Kami changed the title ~~[WIP] Fix st2 login command to work~~ [WIP] Fix st2 login and st2 whoami commands to work Feb 10, 2017

Use ConfigParser from six.moves everywhere.

4a9be54

Matt Oswalt added 2 commits February 12, 2017 16:20

Added a few more tests; removed refs to 'print' mock experiment

ccaf339

Reordered test_auth tests to reflect the order of functions being tested

50c8dc0

Kami commented Feb 13, 2017

View reviewed changes

Merge branch 'master' into fix_st2_login_command

d2ddf10

Merge branch 'fix_st2_login_command' of github.com:StackStorm/st2 int…

8424923

…o fix_st2_login_command

Kami changed the title ~~[WIP] Fix st2 login and st2 whoami commands to work~~ Fix st2 login and st2 whoami commands to work Feb 13, 2017

Kami commented Feb 13, 2017

View reviewed changes

Kami added 5 commits February 13, 2017 09:41

Use StringIO from six.

41090ab

Reduce code duplication and increase re-use.

c1923f6

Reduce code duplication even further.

ba85739

For safety reason use tempfile.

339af89

Simplify code even further - we can just set a default value on the

2602577

class variable.

Kami mentioned this pull request Feb 13, 2017

Remove obsolete coverall badge from readme #3208

Merged

Kami mentioned this pull request Feb 13, 2017

Ignore "index with different options already exists" mongoengine errors #3202

Merged

Kami added 3 commits February 13, 2017 11:08

Merge branch 'master' into fix_st2_login_command

8f03284

Make sure we reset self.stdout and self.stderr on each setUp call

380b958

otherwise it will be re-used when multiple class inherit from this base class which is undesired since we always want to start with a clean state.

Replace CaptureStdout with existing stdout and stderr capture mechanism.

072bd3d

Mierdin approved these changes Feb 13, 2017

View reviewed changes

Mierdin merged commit 74d3dbd into master Feb 13, 2017

Mierdin deleted the fix_st2_login_command branch February 13, 2017 18:47


		class TestLoginUncaughtException(TestLoginBase):

		CONFIG_FILE = '/tmp/logintest.cfg'


		DOTST2_PATH = os.path.expanduser('~/.st2/')

		def __init__(self, args, *kwargs):

Uh oh!

Fix st2 login and st2 whoami commands to work #3211

Fix st2 login and st2 whoami commands to work #3211

Uh oh!

Conversation

Kami commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO

Uh oh!

Kami commented Feb 10, 2017

Uh oh!

Kami commented Feb 10, 2017

Uh oh!

Mierdin commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kami commented Feb 10, 2017

Uh oh!

Mierdin commented Feb 10, 2017

Uh oh!

codecov-io commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Mierdin commented Feb 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kami commented Feb 13, 2017

Uh oh!

Kami Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

Kami commented Feb 13, 2017

Uh oh!

Kami Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

Mierdin Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

Kami Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

Kami Feb 13, 2017

Choose a reason for hiding this comment

Uh oh!

Kami commented Feb 13, 2017

Uh oh!

Kami commented Feb 13, 2017

Uh oh!

Kami commented Feb 13, 2017

Uh oh!

Kami commented Feb 13, 2017

Uh oh!

Mierdin left a comment

Choose a reason for hiding this comment

Uh oh!

Mierdin commented Feb 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Kami commented Feb 10, 2017 •

edited

Loading

Mierdin commented Feb 10, 2017 •

edited

Loading

codecov-io commented Feb 12, 2017 •

edited

Loading

Mierdin commented Feb 12, 2017 •

edited

Loading