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.

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

@stsewd
Copy link
Member

@stsewd 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.

Copy link
Member Author

@stsewd stsewd left a comment

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
Copy link
Member Author

@stsewd stsewd Mar 11, 2018

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).
https://github.com/rtfd/readthedocs.org/blob/e38d16ca1e05effac32bcf9de0175b7ba9085f20/readthedocs/doc_builder/environments.py#L573

@stsewd
Copy link
Member Author

@stsewd stsewd commented Mar 11, 2018

Also, there are missing the same tests for DockerBuildEnvironment

@RichardLitt
Copy link
Member

@RichardLitt 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
Copy link
Member Author

@stsewd 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 Fix tests for environment [RDY] Fix tests for environment Mar 14, 2018
@RichardLitt
Copy link
Member

@RichardLitt 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
Copy link
Member

@humitos humitos commented Mar 14, 2018

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

@RichardLitt
Copy link
Member

@RichardLitt 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
Copy link
Member Author

@stsewd stsewd commented Mar 14, 2018

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

Copy link
Member

@humitos humitos left a comment

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,
Copy link
Member

@humitos humitos Mar 23, 2018

Why not command.exit_code in these?

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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

Copy link
Member

@humitos humitos Mar 23, 2018

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
Copy link
Member

@humitos humitos Mar 23, 2018

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

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

@humitos humitos Mar 23, 2018

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

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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,
Copy link
Member

@humitos humitos Mar 23, 2018

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)

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

#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.

https://github.com/rtfd/readthedocs.org/blob/c5d102a4b119e191fd6a7ea201c776e84dd177f8/readthedocs/doc_builder/environments.py#L436-L445

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
Copy link
Member

@humitos humitos Mar 23, 2018

The comment is incorrect. We are checking build here.

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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)
Copy link
Member

@humitos humitos Mar 23, 2018

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({

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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({
Copy link
Member

@humitos humitos Mar 23, 2018

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

Copy link
Member Author

@stsewd stsewd Mar 23, 2018

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.

Copy link
Member Author

@stsewd stsewd Mar 24, 2018

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 stsewd force-pushed the fix-tests-environment branch 3 times, most recently from af9ab14 to 0a6d1e4 Mar 23, 2018
@stsewd
Copy link
Member Author

@stsewd 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
Copy link
Member

@ericholscher 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 stsewd force-pushed the fix-tests-environment branch from 5be14d3 to 8fc5b6a May 29, 2018
stsewd added 14 commits May 29, 2018
Only recorded commands are added to the list
If the command fails and it's recorded as success the build
does not fail
The command is saved as success,
the original exit code is lost
There is no recorded command
The build is show as success if the command fails but it is
recorded as success.
When the command is not recorded we lose that information
Now the build is recorded as success
@stsewd stsewd force-pushed the fix-tests-environment branch from 8fc5b6a to ef39fe0 May 29, 2018
@stsewd
Copy link
Member Author

@stsewd stsewd commented May 29, 2018

Rebased and checked, everything looks ready

@ericholscher ericholscher merged commit dfed5ea into readthedocs:master May 29, 2018
1 check passed
@stsewd stsewd deleted the 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
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants