Anyway, my first question is whether there’s an appropriate or prefered location for the test.
First, there’s tests.admin_checks.tests, which appears to be the location for all the AdminSite-related tests. (That’s my thought on where it belongs.)
But, the method in question, get_admin_url, is tested for use by readonly foreign key fields in test_readonly_foreignkey_links in tests.admin_views.test.
Then there’s a third idea where the test being created is more directly targeted at just the get_admin_url method within the AdminReadonlyField class and tests this class / method directly. (The other tests test this through the use of the reverse function) There are no tests focusing strictly on this class, so I’m guessing there’s some reason for that - perhaps that it’s considered well-tested within the context of the admin classes.
Thoughts? (Am I overthinking this? Wouldn’t be the first time…)
Nice — Yes, exactly where to put tests, particularly admin tests, is one of the trickier bits.
admin_checks is narrowly for system checks, running at start-up — essentially catching configuration errors — so I don’t think there.
More or less I think that’s correct.
I’d say first option is to add a test next to test_readonly_foreignkey_links (or adjust that, maybe) to check the custom site case — but yes that test is quite high-level. If you want to write a narrower unit-test style case, that’s super. (Improving coverage, or specificity is allowed and welcomed.)
I think the admin.helpers classes are generally covered via admin_views tests, at least at the first pass. So I’d put it in there to begin. It’s eas(y|ier) to see if it looks out of place on a PR.
There is a wider project here, for one in want of a hobby , of curating the test coverage particularly for the admin, so it’s more navigable/comprehensible — but my experience of that is to make small incremental changes, rather than trying to take on a big change, which gets stale and then impossible/inpractical to keep up to date.
I’ve got “something” working. The test fails without my patch and works with my patch. Need to do some cleanup on the code and appropriately comment the test.
Not sure how much I’ll be able to get done tomorrow and will not be able to work on it on Sunday. Expected worse-case scenario is to have a PR prepped by Tuesday.
My branch is (now) ready to issue a pull request. But I can’t figure out at this point what to do to collapse the commits such that you’re only commiting one update.
What do I need to do at this point to make this ready to submit a proper merge request?
This is the point where git leaves orbit, so I’ll give you brief instructions, and expect that you can search for a bit further.
Option 1 git rebase -i
Assuming you have three commits and you want one:
$ git rebase -i HEAD~3
This will open up an $EDITOR where you can pick, reword, squash, &co each commit.
You’ll want to fixup the last two and reword the first one (to have the regulation commit message).
When you save and exit, git will roll-back your commits and reapply them as specified. (i.e. as a single commit.)
Option 2 git reset
git reset --soft HEAD~3
Reset puts the state back to the commit specified (HEAD~3 is “current commit minus three steps”, so the commit your three commits are on top of.)
--soft will keep the changes, but unstage them. (There’s a --medium and a --hard option. There’s no easy way back from --hard)
You can then stage and commit as a single commit.
Once you’ve got it in place, do a git pull --rebase origin main to make sure you’re up to date with any remote changes and you’re ready.
GUIs make rebasing easier (perhaps). I use https://git-fork.com — which is built by a husband and wife team, has a very friendly indefinite evaluation period. Recommended.
I find that the Git Internals chapter takes (some of) the mystery out of it. (The commands are so powerful, they hide that the underlying storage model is rather elegant.)
I hope that’s pitched at the level you’re looking for. Let me know if not.
Ok, even more confused now. I thought my pr (14855) is clean from that perspective? (There may be things needing to be corrected in the code, but that’s a different issue.)
— Yes, exactly where to put tests, particularly admin tests, is one of the trickier bits.
, of curating the test coverage particularly for the admin, so it’s more navigable/comprehensible — but my experience of that is to make small incremental changes, rather than trying to take on a big change, which gets stale and then impossible/inpractical to keep up to date.
)