CI Error on FutureWarning #787
Conversation
adrinjalali
changed the title
|
Would be nice to have this in so that we can merge #745 after |
adrinjalali
changed the title
There was a problem hiding this comment.
This looks good to me, thanks for working on it @adrinjalali!
| pytest --log-cli-level=INFO -sv ./tests/test_repository.py -k RepositoryTest | ||
| pytest -Werror::FutureWarning --log-cli-level=INFO -sv ./tests/test_repository.py -k RepositoryTest |
| @@ -27,7 +27,8 @@ def setUpClass(cls): | |||
| """ | |||
| Share this valid token in all tests below. | |||
| """ | |||
| cls._token = cls._api.login(username=USER, password=PASS) | |||
| cls._token = TOKEN | |||
| cls._api.set_access_token(TOKEN) | |||
There was a problem hiding this comment.
Do we really want to do this? This means that if someone runs the test locally, they will be changing their credential store no?
There was a problem hiding this comment.
Yes it does, but that's already the case (which I think shouldn't be). I'll open a separate issue to discuss that.
There was a problem hiding this comment.
I don't think that's true. login does not update the git credential store, it just gives you the token.
There was a problem hiding this comment.
login stores username and password, just not the token. I don't think either of them should: #796
def login(self, username: str, password: str) -> str:
"""
Call HF API to sign in a user and get a token if credentials are valid.
Outputs: token if credentials are valid
Throws: requests.exceptions.HTTPError if credentials are invalid
"""
warnings.warn(
"HfApi.login: This method is deprecated in favor of `set_access_token`"
" and will be removed in v0.7.",
FutureWarning,
)
path = f"{self.endpoint}/api/login"
r = requests.post(path, json={"username": username, "password": password})
r.raise_for_status()
d = r.json()
write_to_credential_store(username, password)
return d["token"]
def set_access_token(access_token: str):
write_to_credential_store(USERNAME_PLACEHOLDER, access_token)There was a problem hiding this comment.
Ah thank you for pointing it out!
This PR makes the CI to fail on
FutureWarningso that our test suite won't use our own deprecated API and our codebase use new API from our dependencies.