[RDY] Fix tests for environment #3764

stsewd · 2018-03-11T09:16:47Z

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

stsewd · 2018-03-11T09:43:31Z

readthedocs/rtd_tests/tests/test_doc_building.py

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


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 · 2018-03-11T09:45:12Z

Also, there are missing the same tests for DockerBuildEnvironment

RichardLitt · 2018-03-14T03:47:01Z

@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 · 2018-03-14T03:54:52Z

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

RichardLitt · 2018-03-14T04:00:45Z

@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 · 2018-03-14T17:44:53Z

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

RichardLitt · 2018-03-14T19:09:51Z

@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 · 2018-03-14T19:18:11Z

@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 ;)

humitos · 2018-03-23T15:52:29Z

readthedocs/rtd_tests/tests/test_doc_building.py

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


Why not command.exit_code in these?

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

Makes sense. You are right

humitos · 2018-03-23T15:55:42Z

readthedocs/rtd_tests/tests/test_doc_building.py

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


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

humitos · 2018-03-23T16:02:04Z

readthedocs/rtd_tests/tests/test_doc_building.py

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


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

A lot of tests change with that p:

humitos · 2018-03-23T16:06:42Z

readthedocs/rtd_tests/tests/test_doc_building.py

@@ -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,


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)

#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

humitos · 2018-03-23T16:07:29Z

readthedocs/rtd_tests/tests/test_doc_building.py

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


The comment is incorrect. We are checking build here.

I think the assert is wrong

humitos · 2018-03-23T16:09:34Z

readthedocs/rtd_tests/tests/test_doc_building.py

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


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

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.

humitos · 2018-03-23T16:10:10Z

readthedocs/rtd_tests/tests/test_doc_building.py

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


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

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 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 · 2018-03-23T23:54:34Z

@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 · 2018-05-24T20:06:53Z

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

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 · 2018-05-29T17:23:18Z

Rebased and checked, everything looks ready

stsewd commented Mar 11, 2018

View reviewed changes

RichardLitt added the PR: work in progress Pull request is not ready for full review label Mar 14, 2018

stsewd changed the title ~~Fix tests for environment~~ [RDY] Fix tests for environment Mar 14, 2018

RichardLitt added PR: ready for review and removed PR: work in progress Pull request is not ready for full review labels Mar 14, 2018

humitos requested changes Mar 23, 2018

View reviewed changes

stsewd force-pushed the fix-tests-environment branch 3 times, most recently from af9ab14 to 0a6d1e4 Compare March 23, 2018 23:08

stsewd mentioned this pull request Mar 24, 2018

Stable versions are broken #3842

Closed

stsewd mentioned this pull request Mar 31, 2018

Refactor BaseEnvironment #3687

Closed

stsewd force-pushed the fix-tests-environment branch from 861a33a to 5be14d3 Compare March 31, 2018 19:46

stsewd mentioned this pull request Apr 12, 2018

Log an error when the Build.pk differs from the pk in Build.error message #3940

Closed

stsewd force-pushed the fix-tests-environment branch from 5be14d3 to 8fc5b6a Compare May 29, 2018 15:47

stsewd added 6 commits May 29, 2018 10:56

Expose wrong tests

6cac359

Add additional asserts

1c1e60e

Fix tests

8313078

Expose falling assert

4dafcf6

Add tests for DockerBuildEnvironment

2d1df13

Fix tests

49df30e

stsewd added 14 commits May 29, 2018 10:56

Add test for record=False

0739ec6

Add test record_as_success

bf4a181

Expose failed test

35fdca4

Fix falling test

cdb15c0

Better comments

5c15826

Use literal unicode strings

880a863

Better comment

f1f7d7e

Fix test

38fede0

Only recorded commands are added to the list

Fix test

81ad90d

If the command fails and it's recorded as success the build does not fail

Fix test

4b6a64e

The command is saved as success, the original exit code is lost

Fix test

09a2487

There is no recorded command

Fix test

04308a6

The build is show as success if the command fails but it is recorded as success.

Fix test

52c6875

When the command is not recorded we lose that information

Fix tests

ef39fe0

Now the build is recorded as success

stsewd force-pushed the fix-tests-environment branch from 8fc5b6a to ef39fe0 Compare May 29, 2018 15:56

ericholscher merged commit dfed5ea into readthedocs:master May 29, 2018

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RDY] Fix tests for environment #3764

[RDY] Fix tests for environment #3764

stsewd commented Mar 11, 2018

stsewd left a comment

stsewd Mar 11, 2018 •

edited

Loading

stsewd commented Mar 11, 2018

RichardLitt commented Mar 14, 2018 •

edited

Loading

stsewd commented Mar 14, 2018

RichardLitt commented Mar 14, 2018

humitos commented Mar 14, 2018

RichardLitt commented Mar 14, 2018

stsewd commented Mar 14, 2018

humitos left a comment

humitos Mar 23, 2018

stsewd Mar 23, 2018

humitos Mar 23, 2018

humitos Mar 23, 2018

humitos Mar 23, 2018

stsewd Mar 23, 2018

humitos Mar 23, 2018

stsewd Mar 23, 2018 •

edited

Loading

humitos Mar 23, 2018

stsewd Mar 23, 2018

humitos Mar 23, 2018

stsewd Mar 23, 2018

humitos Mar 23, 2018

stsewd Mar 23, 2018

stsewd Mar 24, 2018

stsewd commented Mar 23, 2018

ericholscher commented May 24, 2018

stsewd commented May 29, 2018

[RDY] Fix tests for environment #3764

[RDY] Fix tests for environment #3764

Conversation

stsewd commented Mar 11, 2018

stsewd left a comment

Choose a reason for hiding this comment

stsewd Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

stsewd commented Mar 11, 2018

RichardLitt commented Mar 14, 2018 • edited Loading

stsewd commented Mar 14, 2018

RichardLitt commented Mar 14, 2018

humitos commented Mar 14, 2018

RichardLitt commented Mar 14, 2018

stsewd commented Mar 14, 2018

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd Mar 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stsewd commented Mar 23, 2018

ericholscher commented May 24, 2018

stsewd commented May 29, 2018

stsewd Mar 11, 2018 •

edited

Loading

RichardLitt commented Mar 14, 2018 •

edited

Loading

stsewd Mar 23, 2018 •

edited

Loading