test_provenpackager_request_privs has all kinds of issues

[AdamWill]

test_provenpackager_request_privs has all kinds of issues

[mattiaverga]

As I read the unit test, it is:

bodhi/bodhi-server/tests/services/test_updates.py

Lines 1546 to 1558 in f610256

	def test_provenpackager_request_privs(self, *args):
	"Ensure provenpackagers can change the request for any update"
	nvr = 'bodhi-2.1-1.fc17'
	user = User(name='bob')
	user2 = User(name='ralph')
	self.db.add(user)
	self.db.add(user2) # Add a packager but not proventester
	self.db.add(User(name='someuser')) # An unrelated user with no privs
	self.db.commit()
	group = self.db.query(Group).filter_by(name='provenpackager').one()
	user.groups.append(group)
	group2 = self.db.query(Group).filter_by(name='packager').one()
	user2.groups.append(group2)

creates three users: bob is a provenpackager, ralph is a normal packager, someuser is neither of both. (yes, the use of proventester is odd, I assume it refers to old terminology)

bodhi/bodhi-server/tests/services/test_updates.py

Lines 1560 to 1572 in f610256

	with mock.patch('bodhi.server.Session.remove'):
	app = TestApp(main({}, testing='ralph', session=self.db, **self.app_settings))
	up_data = self.get_update(nvr)
	up_data['csrf_token'] = app.get('/csrf').json_body['csrf_token']
	with fml_testing.mock_sends(update_schemas.UpdateReadyForTestingV3,
	update_schemas.UpdateRequestTestingV1):
	res = app.post_json('/updates/', up_data)
	assert 'does not have commit access to bodhi' not in res
	build = self.db.query(RpmBuild).filter_by(nvr=nvr).one()
	build.update.test_gating_status = TestGatingStatus.passed
	assert build.update.request == UpdateRequest.testing

Creates an update by using user ralph.

bodhi/bodhi-server/tests/services/test_updates.py

Lines 1574 to 1588 in f610256

	# Try and submit the update to stable as a non-provenpackager
	with mock.patch('bodhi.server.Session.remove'):
	app = TestApp(main({}, testing='ralph', session=self.db, **self.app_settings))
	post_data = dict(update=nvr, request='stable',
	csrf_token=app.get('/csrf').json_body['csrf_token'])
	res = app.post_json(f"/updates/{res.json['alias']}/request", post_data, status=400)
	# Ensure we can't push it until it meets the requirements
	assert res.json_body['status'] == 'error'
	assert res.json_body['errors'][0]['description'] == config.get('not_yet_tested_msg')
	update = Build.query.filter_by(nvr=nvr).one().update
	assert update.stable_karma == 3
	assert update.locked is False
	assert update.request == UpdateRequest.testing

ralph tries to submit the update to stable, but they cannot because the update has not yet reached any threshold (and it is still in pending -> testing).

bodhi/bodhi-server/tests/services/test_updates.py

Lines 1590 to 1613 in f610256

	# Pretend it was pushed to testing
	update.request = None
	update.status = UpdateStatus.testing
	update.pushed = True
	# Clear pending messages
	self.db.info['messages'] = []
	self.db.commit()
	assert update.karma == 0
	update.comment(self.db, "foo", 1, 'foo')
	update = Build.query.filter_by(nvr=nvr).one().update
	assert update.karma == 1
	assert update.request is None
	update.comment(self.db, "foo", 1, 'bar')
	update = Build.query.filter_by(nvr=nvr).one().update
	assert update.karma == 2
	assert update.request is None
	with fml_testing.mock_sends(api.Message, api.Message, api.Message):
	update.comment(self.db, "foo", 1, 'biz')
	update = Build.query.filter_by(nvr=nvr).one().update
	assert update.karma == 3
	assert update.request == UpdateRequest.stable
	# Let's clear any messages that might get sent
	self.db.info['messages'] = []

Force the update into testing and post three comments with karma so that the autopush is triggered.

bodhi/bodhi-server/tests/services/test_updates.py

Lines 1615 to 1644 in f610256

	# Set it back to testing
	update.request = UpdateRequest.testing
	# Try and submit the update to stable as a proventester
	with mock.patch('bodhi.server.Session.remove'):
	app = TestApp(main({}, testing='bob', session=self.db, **self.app_settings))
	with fml_testing.mock_sends(update_schemas.UpdateRequestStableV1):
	res = app.post_json(f'/updates/{update.alias}/request',
	dict(update=nvr, request='stable',
	csrf_token=app.get('/csrf').json_body['csrf_token']),
	status=200)
	assert res.json_body['update']['request'] == 'stable'
	with mock.patch('bodhi.server.Session.remove'):
	app = TestApp(main({}, testing='bob', session=self.db, **self.app_settings))
	with fml_testing.mock_sends(update_schemas.UpdateRequestObsoleteV1):
	res = app.post_json(f'/updates/{update.alias}/request',
	dict(update=nvr, request='obsolete',
	csrf_token=app.get('/csrf').json_body['csrf_token']),
	status=200)
	assert res.json_body['update']['request'] is None
	# We need to re-fetch the update from the database since the post calls committed the
	# transaction.
	update = Build.query.filter_by(nvr=nvr).one().update
	assert update.request is None
	assert update.status == UpdateStatus.obsolete

Move the update back to testing, like autokarma was not enabled, and use bob user to push to stable and then obsolete it. As a provenpackager they must can do that, also the Update has reached the karma threshold.

The following is just a check that bob and ralph can edit the update, other users can't.

[AdamWill]

Yes, that's how I read it too - but my point is, none of that makes a lot of sense.

yes, the use of proventester is odd, I assume it refers to old terminology

well, it's both old and wrong. proventester is a real thing, separate from provenpackager, which at one point had different powers (proventester feedback was more important than regular user feedback). That's been disabled approximately forever, but some of the code is still around. So saying proventester is very confusing.

ralph tries to submit the update to stable, but they cannot because the update has not yet reached any threshold (and it is still in pending -> testing).

Yes. Why are we doing this? What is it testing that has anything to do with "proven packager request privileges", especially since we do not check whether bob can submit the update in the same situation?

Force the update into testing and post three comments with karma so that the autopush is triggered.

Yes. Why? I don't know. Do you?

Move the update back to testing, like autokarma was not enabled

Well, yeah, except it doesn't do that. A normal update in testing has a request of None or UpdateRequest.stable. It doesn't make any sense for an update in testing to have a request of UpdateRequest.testing. Also, it's not really "moving the update back to testing", because it doesn't change its status. Really, the test relies on the fact that the update never actually reached stable because the autopush only sets the request, and we don't run the composer. But the test doesn't make the situation at all clear. If we pretend all the other behaviour made any sense, this should be something like:

# Set the request back to None (the "autopush" only changed the request, not the status yet)
update.request = None

use bob user to push to stable and then obsolete it. As a provenpackager they must can do that

Right. Note this is the first part of the test, after Pete knows how many lines, where we actually assert anything about provenpackager privileges at all. But if all we wanted to do was assert that a provenpackager could submit a stable request for a package they don't own, we could do it in way less code, without involving a named user called ralph or someuser, and without ever touching the autopush mechanism. All we need is an update owned by someone other than bob, and two positive comments.

also the Update has reached the karma threshold

yes. But why do we only test ralph's capabilities before this happens, and only test bob's capabilities after it happens? That doesn't seem to make any sense.

The following is just a check that bob and ralph can edit the update, other users can't.

Sure. In fact it's a relatively sane test! But it doesn't need any of the stuff that happened before it, and it has nothing to do with request privileges. It should be a separate test (or the test should be renamed to make it clearer it's testing provenpackager abilities in general).

So ultimately, we agree this huge long test boils down to only three assertions about 'request privileges' at all:

1. A non-provenpackager packager who owns an update cannot push it stable if it hasn't reached the minimum karma or time requirements for manual push (we already have tests of this elsewhere, anyway)
2. A provenpackager who does not own an update can push it stable if it has reached the minimum karma requirement for manual push (only we take an extremely over-complicated route to get here)
3. A provenpackager who does not own an update can obsolete it

The first is tested elsewhere, and probably 75% of the test isn't necessary for the second. We never test the obvious comparisons: can a provenpackager push an update stable without karma? And can a non-provenpackager who does not own an update push it stable or obsolete it? We never test someuser's capabilities regarding requests at all (only edits).

And of course, if we want this to be a test of "all provenpackager capabilities", we're missing a bunch of stuff. There are other tests for provenpackager capabilities around it, so stuffing non-request-related stuff into this test doesn't seem to make much sense. There is a test_provenpackager_edit_anything directly above it which seems like a much more sensible place to put assertions about editing abilities.

To me, the test kinda reads like it wanted to test that provenpackagers can push updates stable without karma, only it never actually did that.

[mattiaverga]

Force the update into testing and post three comments with karma so that the autopush is triggered.

Yes. Why? I don't know. Do you?

I assume it was meant to reach the minimum karma threshold to be able to manually push the update, but it got confused with the autopush threshold. Probably we just need two positive comments without the need of triggering the autopush. I don't remember if it was me writing that code...