So I did a lot of boilerplate work for the tests here with the intent of adding several to test each function and variations on the URL (and anything else that looks interesting to test, in the email contents).

But before I do that I'd like someone's blessing for the general approach, so I don't waste any more time on it if you don't like it. In a few ways the test comes a bit close to just reproducing the tested function, it's a tough call where to just use the same attribute the tested function will pass and where to include its expected value (e.g. user names and email addresses and such). If anyone has thoughts...

If we wanted to focus the tests a bit more strictly on just checking the URLs are correct, an alternative approach (the one I started with) is to mock send_email with a function that just stores the args it's called with into the test class, then we can trivially check that the expected URL is in the email text and not bother about the rest of it.

okay, so now I understand the design I see why we can't trivially include the URLs for the models in that code - the db code has no access to the app so we can't simply use the app routes. We'd have to effectively duplicate them into the db code, which is not exactly a win.

I guess whether it's worth doing anything about changing the URL construction depends on if there are any other bits of Pagure which are doing something similar and trying to reconstruct URLs for DB models from their properties; if so we could throw some convenience functions into pagure.lib or something.

Maybe the expected text could be entirely hard-coded no?

One comment for the test but otherwise looks good to me as is.

I agree, it's fine to hardcode tests, especially when the test is deterministic as this appears to be.

LGTM. I think whether you choose to hardcode the tests or not is optional, but I defer to @pingou on that. If it were me, I'd just hardcode it to keep it simpler and more readable.

the only issue with hardcoding it, really, is if the fixtures change. but then it shouldn't be too hard to adapt the test. so I guess I'll do that. but I'm working on the other PR atm.

So, here's another version. All changes are to the tests.

The tests are now somewhat more hardcoded, and I tweaked the way the fixtures are set up. I also split out the assert_called_with into individual checks for specific args, as it's much easier to see what's wrong when the test fails this way. I added enough tests to cover the possible URL variants (so far). We could add more tests to check stuff like the correct mail recipients in various cases, but I've already spent a lot of time on this and can't really justify spending more on that.

I also did some pylint cleanup.

pylint points up one interesting issue I was reluctant to 'fix'. tests/__init__.py does this:

self.session = pagure.lib.model.create_tables(
        DB_PATH, acls=pagure.APP.config.get('ACLS', {}))

That does not actually return a session instance, but a factory function that SQLAlchemy calls a 'registry'. This is discussed here and documented upstream.

Through some wizardry doing self.session.add(), self.session.commit() etc. works, but it confuses pylint, which complains that instances of scoped_session don't have that member, because technically they don't.

We could 'solve' this by calling the factory function when assigning self.session; then we actually get a session instance which really has the members in question, so pylint is happy. However, I'm not sure of the precise implications of doing this, so I'm not going to do it in this PR. It's worth someone with a better grasp of the SQL and threading stuff looking at it, though. Note that upstream's example does:

Session = scoped_session(session_factory)
some_session = Session()

our code is kinda missing that second step.

Obviously, if we were gonna change this somehow, we'd probably want to change the similar bits in the actual Pagure code, not just the tests...

I think we could also stand to add a forked project to the 'fixtures' you get from tests.create_projects() and adjust all the tests that use a forked project to use that (or as many of them as feasible), but again, I can't really spare the time to do that myself, sorry...

I'm going to merge this PR as is, many thanks for working on this and spending that much time on it!

Your suggestions around the session and the tests are probably worth putting in their own ticket so we're just they do not get lost. Would you like to do it? Otherwise I will :)

Thanks again!

After poking around at the scoped_session stuff some more I think I think it's not really wrong, so instead, I came up with a PR to shut pylint up in a better way than we were previously doing it: #1565