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.

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

[RDY] Fix tests for environment #3764

Merged
merged 20 commits into from May 29, 2018

Conversation

Projects
None yet
4 participants
@stsewd
Member

stsewd commented Mar 11, 2018

While refactoring #3687 I discovered some missing asserts in the tests and also one test isn't testing at all.

@stsewd

This commit is just to show you on travis that the tests just pass when shouldn't

with build_env:
build_env.update_build(BUILD_STATE_CLONING)
# FIXME: This method raise an assert exception,
# but is captured by the context manager

This comment has been minimized.

@stsewd

stsewd Mar 11, 2018

Member

Also I found this hidden falling tests, the context manager covered it.
If we try to check the mock_calls outside the context manager the tests will fail anyway since we are passing a reference here, which is captured by the mock, i.e at the end we have two calls (one from the original update, and one from the __exit__) with the same reference to the dict (the values from the first call are lost).

api_v2.build(self.build['id']).put(self.build)

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 11, 2018

Also, there are missing the same tests for DockerBuildEnvironment

@RichardLitt

This comment has been minimized.

Member

RichardLitt commented Mar 14, 2018

@stsewd It's pretty hard for me sometimes to know when a PR is ready to be reviewed when you stagger commits like this. Can you start including a [WIP] note in the title, or in the main comment, and then signify when it's ready for review in a comment?

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 14, 2018

@RichardLitt sorry, I add notes for my self sometimes. Sure, I will start to using the [wip] and [rdy] tags on my PRs. This one is ready for a review.

@stsewd stsewd changed the title from Fix tests for environment to [RDY] Fix tests for environment Mar 14, 2018

@RichardLitt

This comment has been minimized.

Member

RichardLitt commented Mar 14, 2018

@stsewd No worries! I'm just trying to make it easier for other reviewers to know that you're ready to go or not. :)

@humitos

This comment has been minimized.

Member

humitos commented Mar 14, 2018

@RichardLitt isn't @stsewd allowed to add the proper label to the PR that he opens?

@RichardLitt

This comment has been minimized.

Member

RichardLitt commented Mar 14, 2018

@humitos That would make the most sense, but I haven't seen @stsewd label things yet and I don't know if he has access to that ability at the moment.

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 14, 2018

@RichardLitt @humitos I don't have the necessary permissions

@humitos

Weee! I like all these tests.

Be careful when using the mock library since it's very easy to make mistakes that will pass without raising any exception but in the end we are not testing exactly what we want.

(be sure to rebase your branch to use the latest mock lib --2.0-- since I had the same problem when using assert_ and the test was passing but it wasn't asserting anything :) )

Please, take a look at the comments that I left and re-check to see if the tests are testing exactly what you want (this code is tricky) and I will review it again after that.

Nice work! This is something that we really need ;)

'command': command.get_command(),
'description': command.description,
'output': command.output,
'exit_code': 0,

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Why not command.exit_code in these?

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

I wanna be explicit about the exit code (and also because this change on others tests). Should I keep it?

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Makes sense. You are right

@@ -180,6 +202,8 @@ def test_failing_execution_with_caught_exception(self):
# api() is not called anymore, we use api_v2 instead
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
# The command was not saved

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Change the message to something like: The build failed before executing any command

'output': u'',
'state': u'finished',
'builder': mock.ANY,
'exit_code': 1,

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

After the change I did the other day, this should probably be 0 now.

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

A lot of tests change with that p:

@@ -111,7 +210,6 @@ def test_incremental_state_update_with_no_update(self):
self.mocks.mocks['api_v2.build']().put.assert_called_with({
'id': DUMMY_BUILD_ID,
'version': self.version.pk,
'success': True,

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Mmm...

Why it was passing before you remove this line?

I think that we have to check here that after we exit the with context, we have done a call to put with the sucess=True in the first build_env and we didn't call it in the second one (update_on_success=False)

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

#3764 (comment) (this also explains why we can't make that validation outside the context manager)

Was because the context manager capture the assert exception to exit the context gracefully.

def handle_exception(self, exc_type, exc_value, _):
"""
Exception handling for __enter__ and __exit__
This reports on the exception we're handling and special cases
subclasses of BuildEnvironmentException. For
:py:class:`BuildEnvironmentWarning`, exit this context gracefully, but
don't mark the build as a failure. For all other exception classes,
including :py:class:`BuildEnvironmentError`, the build will be marked as
a failure and the context will be gracefully exited.

So, now I'm checking if the context manager is exceptions free https://github.com/rtfd/readthedocs.org/pull/3764/files#diff-6d379045f32beb719b67ece38d86bf74R222

})
self.assertIsNone(build_env.failure)
# No commands were saved

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

The comment is incorrect. We are checking build here.

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

I think the assert is wrong

})
self.assertIsNone(build_env.failure)
# No commands were saved
self.assertFalse(self.mocks.mocks['api_v2.build'].post.called)

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

I think there is a problem here.

This method should have been called (api_v2.build) since it's the one that has updated our builds inside the with context.

I noticed that maybe you missed to add the () to the mocks. Take a look a the other lines that use the same call:

self.mocks.mocks['api_v2.build']().put.assert_called_with({

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

Of what I remember from that long afternoon of debugging, the put method was used to save a build. I going to take a more closer look to the code.

@@ -147,6 +247,17 @@ def test_failing_execution(self):
# api() is not called anymore, we use api_v2 instead
self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
# The command was saved
command = build_env.commands[0]
self.mocks.mocks['api_v2.command'].post.assert_called_once_with({

This comment has been minimized.

@humitos

humitos Mar 23, 2018

Member

Please, check all of these lines because maybe you are missing the () in them.

This comment has been minimized.

@stsewd

stsewd Mar 23, 2018

Member

I think this was because the put method was getting called with an id on () before sending the data, the post method just send the data.

This comment has been minimized.

@stsewd

stsewd Mar 24, 2018

Member

This is from pdb (post and put methods)

248             # api() is not called anymore, we use api_v2 instead
249             self.assertFalse(self.mocks.api()(DUMMY_BUILD_ID).put.called)
250             # The command was saved
251             command = build_env.commands[0]
252             __import__('pdb').set_trace()
253  ->         self.mocks.mocks['api_v2.command'].post.assert_called_once_with({
254                 'build': DUMMY_BUILD_ID,
255                 'command': command.get_command(),
256                 'description': command.description,
257                 'output': command.output,
258                 'exit_code': 1,
(Pdb++) mock = self.mocks.mocks['api_v2.command']
(Pdb++) dir(mock)
['assert_any_call', 'assert_called', 'assert_called_once', 'assert_called_once_with', 'asser
t_called_with', 'assert_has_calls', 'assert_not_called', 'attach_mock', 'call_args', 'call_a
rgs_list', 'call_count', 'called', 'configure_mock', 'get', 'method_calls', 'mock_add_spec',
 'mock_calls', 'post', 'reset_mock', 'return_value', 'side_effect']
(Pdb++) mock.post.call_args_list
[call({'start_time': datetime.datetime(2018, 3, 24, 0, 3, 39, 202898), 'command': u'echo tes
t', 'output': u'This is not okay', 'exit_code': 1, 'description': '', 'end_time': datetime.d
atetime(2018, 3, 24, 0, 3, 39, 203003), 'build': 123})]
(Pdb++) mock().post.call_args_list
[]
(Pdb++) mock = self.mocks.mocks['api_v2.build']
(Pdb++) mock.put.call_args_list
[]
(Pdb++) mock().put.call_args_list
[call({'length': 0, 'state': u'finished', 'output': u'', 'exit_code': 1, 'error': u'', 'vers
ion': 20, 'setup_error': u'', 'builder': u'stsewd', u'id': 123, 'project': 6, 'setup': u'',
'success': False})]

@stsewd

This comment has been minimized.

Member

stsewd commented Mar 23, 2018

@humitos please take a look at the latest messages from those commits #3764 (commits), to be sure that this is really what we want (we are losing some information with the recent changes).

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 24, 2018

@stsewd want to rebase this and confirm it still passes. It looks like good stuff that we should have in master.

stsewd added some commits Mar 23, 2018

Fix test
Only recorded commands are added to the list
Fix test
If the command fails and it's recorded as success the build
does not fail
Fix test
The command is saved as success,
the original exit code is lost
Fix test
There is no recorded command
Fix test
The build is show as success if the command fails but it is
recorded as success.
Fix test
When the command is not recorded we lose that information
Fix tests
Now the build is recorded as success
@stsewd

This comment has been minimized.

Member

stsewd commented May 29, 2018

Rebased and checked, everything looks ready

@ericholscher ericholscher merged commit dfed5ea into rtfd:master May 29, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:fix-tests-environment branch May 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment