#3089 More flakiness fixes
Merged 4 years ago by pingou. Opened 4 years ago by abompard.
abompard/pagure tests-flakiness  into  master

@@ -1767,8 +1767,8 @@ 

              [u'code', u'message']

          )

          self.assertEqual(js_data['code'], 'OK')

-         self.assertEqual(

-             js_data['message'].keys(),

+         self.assertListEqual(

+             sorted(js_data['message'].keys()),

              [u'branch_w_pr', u'new_branch'])

          self.assertEqual(js_data['message']['branch_w_pr'], {})

          self.assertEqual(js_data['message']['new_branch'].keys(), ['feature'])
@@ -1893,7 +1893,7 @@ 

          )

          self.assertEqual(js_data['code'], 'OK')

          self.assertEqual(

-             js_data['message'].keys(),

+             sorted(js_data['message'].keys()),

              [u'branch_w_pr', u'new_branch'])

          self.assertEqual(js_data['message']['branch_w_pr'], {})

          self.assertEqual(js_data['message']['new_branch'].keys(), ['feature'])

@@ -592,8 +592,11 @@ 

          self.assertEqual(output.status_code, 404)

  

      @patch('pagure.lib.notify.send_email')

-     def test_request_pull_empty_repo(self, send_email):

+     @patch('pagure.lib.git.update_pull_ref')

+     def test_request_pull_empty_repo(self, send_email, update_pull_ref):

          """ Test the request_pull endpoint against an empty repo. """

+         # Mock update_pull_ref or the repo won't be empty anymore

But the repo was empty when the PR got created (granted, not after) :)

+         # (the PR will have been pushed to refs/pull)

          send_email.return_value = True

  

          tests.create_projects(self.session)
@@ -672,6 +675,7 @@ 

              '  PR from the feature branch\n</h3>', output.data)

          self.assertTrue(

              output.data.count('<span class="commitdate" title='), 1)

+         update_pull_ref.assert_called()

  

          shutil.rmtree(newpath)

  
@@ -988,8 +992,11 @@ 

          self.assertEqual(patch, exp)

  

      @patch('pagure.lib.notify.send_email')

-     def test_request_pull_patch_empty_repo(self, send_email):

+     @patch('pagure.lib.git.update_pull_ref')

+     def test_request_pull_patch_empty_repo(self, send_email, update_pull_ref):

          """ Test the request_pull_patch endpoint against an empty repo. """

+         # Mock update_pull_ref or the repo won't be empty anymore

+         # (the PR will have been pushed to refs/pull)

          send_email.return_value = True

  

          tests.create_projects(self.session)

@@ -302,23 +302,11 @@ 

          self.assertEqual(item.user, 'foouser')

          self.assertTrue(item.password.startswith('$2$'))

  

-         # I'm not sure if the change was in flask or werkzeug, but in older

-         # version flask.request.remote_addr was returning None, while it

-         # now returns 127.0.0.1 making our logic pass where it used to

-         # partly fail

-         if hasattr(flask, '__version__'):

-             flask_v = tuple(int(el) for el in flask.__version__.split('.'))

-             if flask_v <= (0, 12, 0):

-                 self.assertIn(

-                     '<a class="nav-link btn btn-primary" '

-                     'href="/login/?next=http://localhost/">', output.data)

-                 self.assertIn(

-                     'Could not set the session in the db, please report '

-                     'this error to an admin', output.data)

-             else:

-                 self.assertIn(

-                     '<a class="dropdown-item" '

-                     'href="/logout/?next=http://localhost/">', output.data)

+         # We have set the REMOTE_ADDR in the request, so this works with all

+         # versions of Flask.

If I merge this and it fails on ci.centos.org, I'll come back to haunt you, deal? :)

+         self.assertIn(

+             '<a class="dropdown-item" '

+             'href="/logout/?next=http://localhost/">', output.data)

  

      @patch.dict('pagure.config.config', {'PAGURE_AUTH': 'local'})

      @patch('pagure.lib.notify.send_email', MagicMock(return_value=True))

Fixes some more tests in the line of f61bb39.

But the repo was empty when the PR got created (granted, not after) :)

If I merge this and it fails on ci.centos.org, I'll come back to haunt you, deal? :)

About mocking pagure.lib.git.update_pull_ref(), it took me a while to track it down but this function is called by pagure.lib.tasks.sync_pull_ref() which is itself called by pagure.lib.new_pull_request() in the test. At the moment, it's a race condition : it's counting on the fact that the delayed task will not run before we make the GET request. If it runs before, then pagure.lib.git.get_diff_info() will see the orig_repo.is_empty as False and it will blow up.

About setting REMOTE_ADDR, yeah I've tested it on Flask 0.11 and Flask 0.12 and both work.

I'm not quite seeing the race condition since pagure.lib.git.get_diff_info() is called before the PR gets created, so I'm not entirely sure how they could be racing.

hmm nope in the test get_diff_info() is called after, it's called when doing GET on the pull request via the diff_pull_request() function.

Hm, ok I see what you mean, but if that raises an error it's likely something we need to fix as opening a PR against an empty project is a valid use-case

Yes, the consequence is a redirect to the project page with the flash message "Branch master could not be found in the target repo". It comes from get_diff_info(), the .is_empty condition there returns False because the refs/pull isn't empty, but there is still no master branch. Maybe make that condition a bit smarter?

Sounds good, do we want to do it here or merge this and track that in another PR?

Another PR sounds cleaner

Let's rebase and merge :)

rebased onto d90ed46

4 years ago

Pull-Request has been merged by pingou

4 years ago