test_sys_checkout: check that desired repo was actually checked out.

[johnpaulalex]

Also change GitRepository._determine_remote_name() into a classmethod _remote_name_for_url() that takes in a url, to make it more self-evident how the remote name is being picked.

User interface changes?: No

Fixes: None

Testing: test removed: none unit tests: none system tests: 'make stest' passes manual testing: none

[johnpaulalex]

Not sure if I'm actually checking the right thing in this CL - my git knowledge is weak.

Also I'll be sweeping through and changing all the chdir's to git -C in a follow-up CL.

[johnpaulalex]

Ah this is a first - is this 'test manic' failure legit? Given that 'make stest' worked locally, I'm skeptical that this failure is real, but who knows.

[billsacks]

Thanks a lot for adding this check!

Not sure if I'm actually checking the right thing in this CL - my git knowledge is weak.

It looks like the intent is to check if the URL of the remote associated with the name 'origin' matches the URL set in the externals file. If so, I think this is doing the right thing. However, the way this is being checked seems less straightforward than it could be: it took me a little time to understand what it was doing. If I understand the intent correctly, I think a more straightforward way to check this would be to run git remote get-url origin and compare the result with the expected URL. I don't feel like that needs to be changed, but just thought I'd suggest that alternative.

Ah this is a first - is this 'test manic' failure legit? Given that 'make stest' worked locally, I'm skeptical that this failure is real, but who knows.

I think these are real failures. It looks like when you're testing from the command line you may just be running make stest, but some (many) of the tests are executed with make utest (and those are the failures here) – or you can run both with make test.

[johnpaulalex]

Ah TIL, thanks Bill. I was thrown off by the github test output because it decided to auto-expand the very chatty 'setup tmate session' tab, which was full of ssh command lines; I then missed the 'test-manic' tab above it, which had the test output.

All fixed now, the test was doing some monkeypatching that needed updating.

As for the original intent: I wanted to verify that checkout_externals is checking out the right repo. Is checking that the magic remote name 'origin' points to the right url the right way to do this? I had convinced myself before that the answer was 'yes' but now I'm not so sure.

As for the more direct git command to verify what origin points at, I was just reusing the existing _determine_remote_name method (https://github.com/ESMCI/manage_externals/blob/55e74bd0a4ed1b6070633320bd142c954d058b19/manic/repository_git.py#L235), which called 'git remote --verbose' and then parsed the output. I'm happy to replace that method with the command line you're suggesting, in a followup CL, if you think that is a good idea.

[jedwards4b]

The setup tmate session section should only be used for debugging the github workflow and should otherwise be commented out - sorry if I didn't leave it in that state.

[billsacks]

As for the original intent: I wanted to verify that checkout_externals is checking out the right repo. Is checking that the magic remote name 'origin' points to the right url the right way to do this? I had convinced myself before that the answer was 'yes' but now I'm not so sure.

Yes, this seems like the right way to do it, as long as this is an initial (fresh) clone. (If you had an existing clone and were rerunning manage_externals, then it's possible that the remote would be named something different from origin. But for a fresh clone, the remote is named origin... that is theoretically changeable in the git clone command, but I don't think we do that.)

As for the more direct git command to verify what origin points at, I was just reusing the existing _determine_remote_name method

That's fine. It works fine, it's just a bit backwards from what I was expecting to see: It looks like that function says, "what is the remote name associated with a given URL?" and so you're checking if the remote name of the expected URL is "origin", whereas the more straightforward logic (to me, anyway) would be to ask, "what is the URL associated with the remote named 'origin'" and then compare the result to the expected URL. Not a big deal... there's always more cleanup that could be done, but it's probably getting time to call this good enough and move on :-)

Speaking of things that probably shouldn't be fixed now: At some point we might want to replace this monkeypatching with use of unittest.mock.patch, but I don't think that needs to be done now.

[johnpaulalex]

Yes please, all ready to go.