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

OAuth tests cleanup, they don't modify the user's profile anymore #226

Merged
merged 22 commits into from
Aug 7, 2021

Conversation

marioortizmanero
Copy link
Collaborator

@marioortizmanero marioortizmanero commented Jul 14, 2021

Description

This PR takes care of the "Authenticated tests should actually not modify the user's account and cause minimal changes (as in, don't completely delete the user's library for a test)." task in #127.

It also includes some fixes so that they work:

Dependencies

None

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

cargo test --features=env-file,cli -- --ignored --test-threads=1
cargo test --features=env-file,cli,client-ureq,ureq-rustls-tls --no-default-features -- --ignored --test-threads=1

@marioortizmanero marioortizmanero marked this pull request as draft July 14, 2021 19:01
@marioortizmanero
Copy link
Collaborator Author

@thecakeisalie25, remembering your comment in #154, I've just cleaned up the tests with oauth in this PR and I wouldn't really add them to an automatic flow. It's impossible to fully revert all of the changes the tests may apply (the queue can't be restored, for example), and it's still quite annoying if you're playing music at the same time (some songs play for a fraction of a second, etc). I wouldn't like these things happening whenever a new commit reaches master on my personal account. Or at least in its current state.

Check out these tests and see for yourself.

@marioortizmanero marioortizmanero marked this pull request as ready for review July 14, 2021 22:38
src/clients/oauth.rs Show resolved Hide resolved
rspotify-model/src/album.rs Outdated Show resolved Hide resolved
rspotify-model/src/idtypes.rs Show resolved Hide resolved
@marioortizmanero
Copy link
Collaborator Author

As a warning: I'm leaving on a couple weeks of vacation, I'll be back for more stuff!

@marioortizmanero
Copy link
Collaborator Author

I'm back! Is there anything else left in order to merge this?

@ramsayleung
Copy link
Owner

The CI failed :)

@marioortizmanero
Copy link
Collaborator Author

I have no clue why the tests failed, honestly. I've added the RUST_BACKTRACE=1 env variable so that next time this happens it's caught. But I didn't touch the function which made the tests fail, so it's not related to this PR and it can be merged.

@ramsayleung ramsayleung merged commit 93a05a7 into master Aug 7, 2021
@ramsayleung ramsayleung deleted the oauth-tests branch August 7, 2021 12:52
@marioortizmanero marioortizmanero mentioned this pull request Aug 31, 2021
87 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment