PR#3089: More flakiness fixes - pagure

pagure

#3089 More flakiness fixes

Merged 6 years ago by pingou. Opened 6 years ago by abompard.

abompard/pagure tests-flakiness into master

More flakiness fixes

Aurélien Bompard • 6 years ago

d90ed46

tests/test_pagure_flask_internal.py

file modified

+3 -3

		`@@ -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'])`

tests/test_pagure_flask_ui_fork.py

file modified

+9 -2

		`@@ -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`
pingou commented 6 years ago 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)`

tests/test_pagure_flask_ui_login.py

file modified

+5 -17

		`@@ -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.`
pingou commented 6 years ago 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))`

abompard commented 6 years ago

Fixes some more tests in the line of f61bb39.

pingou commented on line 9 of tests/test_pagure_flask_ui_fork.py 6 years ago

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

pingou commented on line 23 of tests/test_pagure_flask_ui_login.py 6 years ago

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

abompard commented 6 years ago

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.

abompard commented 6 years ago

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

pingou commented 6 years ago

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.

abompard commented 6 years ago

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.

pingou commented 6 years ago

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

abompard commented 6 years ago

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?