Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Write unit tests for functions that are already in close-to-final form #6

Open
shreyb opened this issue Nov 20, 2023 · 4 comments
Open
Labels
enhancement New feature or request

Comments

@shreyb
Copy link
Collaborator

shreyb commented Nov 20, 2023

As the title says. For new functions, we should test them, and I leave it to the developer whether or not to do something TDD-like, or write the test afterwards.

@shreyb shreyb added the enhancement New feature or request label Nov 20, 2023
@shreyb shreyb assigned cathulhu and shreyb and unassigned cathulhu and shreyb Nov 20, 2023
@shreyb
Copy link
Collaborator Author

shreyb commented Nov 20, 2023

Possibly unit-testable right now:

  1. get_default_cert_path(): https://github.com/fermitools/Ferry-CLI/blob/master/ferry.py#L8
  2. get_default_capath(): https://github.com/fermitools/Ferry-CLI/blob/master/ferry.py#L15C5

@cathulhu
Copy link
Collaborator

cathulhu commented Nov 22, 2023

So for these two ^ the test successful condition would be that a non empty string is returned for the path?

I'm already doing something similar as part of testing the certificate acquisition process:

Screenshot 2023-11-21 at 10 45 13 PM

Do you think the existence of this path should be tested in the same way? I'm wondering under what conditions might these paths from point 1 and 2 above not exist?

@shreyb
Copy link
Collaborator Author

shreyb commented Nov 22, 2023

The test successful condition would be that the correct string is returned. Based on the name of the function, I'm almost in favor of nixing the check to see if the path exists (in the function, not your tests).

Namely, if a user says "I want to use a cert at the default location", it might be better to have the check for the existence of the file somewhere else, or even allow the tool to error out if the file doesn't exist. That way, we could plug in the eventual code you write to automate getting the correct credential. THAT code can check for the existence of the credential at the right place. What do you think?

@shreyb
Copy link
Collaborator Author

shreyb commented Nov 28, 2023

To Test:

  1. ferry.py:set_auth_from_args(): Both 'token', 'cert', and 'certificate' auth methods - make sure we get the right class returned, as well as that it's instantiated right. Also, that ValueError is properly raised if we try to give a different auth method.
  2. Mock ferry server response.

@cathulhu cathulhu removed their assignment Jan 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants