test_provenpackager_request_privs has all kinds of issues

[AdamWill]

I am filing this issue because if I keep fixing every goddamn thing I come across while I'm trying to work on this pull request from hell, I'll never get done.

I'm trying to consolidate the tests for check_karma_thresholds(). As part of that, I've been disabling bits of it, running the test suite, and seeing what fails.

If you disable the bit that actually autopushes updates when they hit autokarma, test_provenpackager_request_privs (from bodhi-server/tests/services/test_updates.py) fails. So I went to look at why, and...what the hell is this thing?

The immediate reason it fails is that, for some inexplicable reason, there's a block in the middle of the test where it intentionally triggers an auto-push for karma:

    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

but there is no indication why it's doing this. There's no comment, there's no explanation in the original commit that added this test - 73c211d . It doesn't appear to have anything to do with the ostensible purpose of this test.

After it does that, the test does this:

    # Set it back to testing
    update.request = UpdateRequest.testing

which leaves the update in a somewhat odd state - it has +3 karma, its status is UpdateStatus.testing , and its request is UpdateRequest.testing . What?

The test has multiple other issues. It says "proventester" instead of "provenpackager" several times, which is confusing. The comments on the users could be clearer, I'd write it like this:

    self.db.add(user) # bob will be a provenpackager
    self.db.add(user2)  # ralph will be a packager, but not a provenpackager
    self.db.add(User(name='someuser'))  # someuser will have no privs

AFAICT, the test creates the update as ralph. Which is weird, because that will give ralph more or less the same powers as bob regarding this update. It seems like it'd be more useful to have the update owned by someone else, then make sure ralph can't do stuff to it. Or have a fourth user, so we can check the privileges of a packager who's the update owner, a packager who's not the update owner, and a provenpackager.

The test then goes on to try and submit the update to stable as ralph (the update owner), and expect to be rejected because it doesn't meet the testing requirements. OK, sure, but...next it goes on to "Pretend it was pushed to testing", then triggers the autopush to stable, then half-heartedly resets it to testing, then tries submitting it stable as bob (a provenpackager). Wat? If it tried submitting it to stable as ralph at that point, it'd work too! This assertion really isn't proving anything much. Why didn't we trying pushing stable as bob in the same circumstance as we tried pushing it stable as ralph, and vice versa?

Then it re-initializes the app as bob even though it's already bob(?!) and checks it can send an 'obsolete' request. OK, that's fine in itself.

Then it goes off on a weird tangent and starts checking who's allowed to edit the update, even though the test name and docstring claim it's supposed to be a test of setting requests. This bit was added at a later date - the original version in 73c211d didn't have it (but it did have all the other problems).

If I just remove the bit of the test where it intentionally triggers a karma autopush, it starts failing because bob cannot request stable, because the karma requirements aren't met. That, with current update policies and Bodhi code, is correct behaviour - provenpackagers don't have superpowers to push untested updates stable. I'm not sure whether, at the time, provenpackagers were meant to have such powers and that changed, or whether the test was just nonsense from the beginning - there isn't enough context on the original commit to tell. But it means I don't really know what to do with this test, I don't really want to rewrite it from scratch as part of my mostly-unrelated PR.

So, yeah, this thing needs fixing. First decide what it's actually supposed to test, then make it...actually test that. :) If it's intended to be a test that provenpackagers can submit requests on other people's updates (and that non-provenpackagers can't), great, make it more sanely that. All the stuff about who can edit updates should probably be split out.

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