remote: fix remote default call with no arguments

[kaiogu]

remote default command with no arguments now returns default remote instead of
setting default remote to None.

Fixes #2813

• ❗ Have you followed the guidelines in the Contributing to DVC list?

• 📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.

• ❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addresses. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[kaiogu]

Noticed too late that this probably breaks --unset option, I'll fix it.

[ghost]

Thanks a lot for the pull request! Could you please add a test for this?

The rest looks good to me)

[ghost]

@kaiogu, adding some more guidance, the test would be under tests/func/test_remote.py.
We have some legacy tests written with unittest but we are slowly moving to pytest, so please, write it with pytest on mind 😅

You can take a look at other tests to get an idea of how we usually test stuff. In general, there's
a main method that we use to invoke CLI commands. In this case, we are trying to test when dvc remote default is called. Also, to intercept the logs you can use the caplog fixture.
There's another fixture to work with a DVC initialized repository: dvc_repo.

So the test would be something like:

def test_show_default(dvc_repo, caplog):
    assert main(["remote", "default", "foo"]) == 0
    assert main(["remote", "default"]) == 0
    assert "foo" in caplog.text

It uses a DVC repository and run the commands dvc remote default foo and then dvc remote default (making sure that the exit code is 0), finally, it tests weather there was foo (the name of the remote) in the output from the logger.

[pared]

LGTM, we can merge after the test is written.

[kaiogu]

Hi @pared, thanks for the info! I could write a test over the weekend if this isn't that time critical. I've got no experience wit pytest (or unittest) but would be willing to learn.

[pared]

@kaiogu please take your time, if you need our help, feel free to write here, or message us directly on our discord :)

[kaiogu]

@pared @MrOutis This last commit passes the test on my machine but doesn't on Travis CI because Travis seems to be writing to the caplog also. Can I call caplog.clear() on the beginning of the test, or would the solution given by mroutis be preferred (in instead of ==)?

[ghost]

@kaiogu, it is better to use in :)

[efiop]

No need to remove

Suggested change

	def test_show_default(caplog):
	def test_show_default(dvc_repo, caplog):

as it makes the test run in the repo itself instead of in the test env. I think you've removed it because DS was complaining, but it was wrong there 🙂

[kaiogu]

That's right. 😄 Some tool said something about prepending the argument with unused or something like that, but I can't find the message anymore. So should I just keep the argument and let the build fail?

[efiop]

@kaiogu Yes, DS will probably fail, but the tests themselves (run on travis) won't, so we are fine 🙂

[pared]

Ill accept after this dvc_repo is reintroduced. :)

[kaiogu]

@pared Done :)

[efiop]

@MrOutis Or we could call caplog.clear() after dvc remote add, right? And then we could use a more strict assert "foo" == caplog.text. Or am I missing something?

[kaiogu]

So I checked again and if I run the test exactly as recommended in the contributions guidelines it fails on my end to.

@efiop caplog.text returns a str of all log messages, with the logger name, level and message for each. To get only the actual message, caplog.record_tuples seems to be needed for equality checking. It returns a list of log tuples like [(logger_name, log_level, message)]. So I could check for stricter equality only if I compare somethingin a way similar to my last commit: assert "foo" == caplog.record_tuples[0][2].

The above does not work in the test environment though. I tried wrapping the body of the test with with caplog.at_level(logging.INFO): but that doesn't work as there seem to be some test-time-only messages also populating the caplog at that logging level.

Just doing the solution proposed by @MrOutis works, but I'd change foo to something less common to avoid accidentaly passing the test if there is a random "foo" in the log. This solution is ready to push.

Alternatively, the recommendation in the python logging docs is to actually use print statements instead of logging in the call to remote default with no args. This output I could capture with the capsys fixture of pytest, it could check for equality, and wouldn't reside in the same "space" as the actual logging messages. I think this would be the best solution overall, but I can implement what you decide. This might entail some reworking of other parts of the code to keep consistency.

[pared]

How about using caplog.records[0].message?
Seems more readable than record_tuples :)

[pared]

Obsolete after efiop comment

[efiop]

@kaiogu You are absolutely right about print, we should use it. I've recommended logger.info through inertion, as we have a log&print mess in our code(we even have a separate issue to make things use print instead of info where appropriate). Indeed, let's use print here instead. And indeed, in that case the test would be able to simply use capsys(as in other our tests). So we only need to change:
logger.info("{}".format(name)) -> print(name) and assert "foo" == caplog.record_tuples[0][2] -> assert capsys.readouterr().out == "foo\n" . I'm sure that is going to work and won't require any additional adjustments in other tests 🙂

[efiop]

Perfect! Thank you so much! 🙏

[kaiogu]

Thank you for the guidance! Happy to contribute! 😄

[efiop]

@kaiogu 0.71.0 is out, please upgrade and give it a try 🙂 Thank you!