inflatablefriends / lastfm

Portable .Net library for Last.fm
Other
100 stars 61 forks source link

Integration tests failing? #146

Closed rikkit closed 1 year ago

rikkit commented 5 years ago

As noticed by @klinge: https://github.com/inflatablefriends/lastfm/pull/143#issuecomment-459347026

klinge commented 5 years ago

I had a look att the failing test ScrobblesSingle. The error is an assert that the actual and expected time the track was played is the same (expected = trackPlayed, actual = scrobbledTrack.TimePlayed). When running test locally with "dotnet test" and on the appveyor build these are off by a time between a few minutes to a few seconds.

But when debuggning the test I get the exact same value for both and the test is not failing. Tried doing this repeatedly with the same results every time..

Error message when running with dotnet test:

Starting test execution, please wait...
Failed   ScrobblesSingle
Error Message:

Differences:
1E: 2019-02-04 12:13:36.00
1A: 2019-02-04 12:10:48.00

  String lengths are both 22. Strings differ at index 15.
  Expected: "2019-02-04 12:13:36.00"
  But was:  "2019-02-04 12:10:48.00"
  --------------------------^
klinge commented 5 years ago

Also looked at the error in UpdatesNowPlaying. This is an error because it seems LastFM now returns an array of ArtistImages in the track object.

A number of other parameters in the actual result are set to null in the response from LastFM to avoid errors when implementation of the API changes. My proposed fix would be to set "actual.ArtistImages = null".

klinge commented 5 years ago

Ran the tests today again. The error on the timestamp on the scrobbledTrack.TimePlayed is still there. Only change is that now it's off by more than 5 seconds.

I also debugged the tests to see how long the Lastfm.Scrobbler.ScrobbleAsync(testScrobble); takes to execute. It's not super fast but the result is returned in less than a second. So it cannot really expain the 5 second error. Could time settings on the LastFM servers be off?

Would it be ok to disable this test for the time being?

rikkit commented 5 years ago

Could it be that there isn't a delay - that a previous scrobble is being used for the comparison? It would explain why the difference in timestamps is so variable.

klinge commented 5 years ago

Good thinking. Will check up on that.

dogguts commented 5 years ago

Also looked at the error in UpdatesNowPlaying. This is an error because it seems LastFM now returns an array of ArtistImages in the track object.

A number of other parameters in the actual result are set to null in the response from LastFM to avoid errors when implementation of the API changes. My proposed fix would be to set "actual.ArtistImages = null".

caused by c3aff5aa29caacf42c3e250d588bdf86ccfab32d Wich expands LastTrack with 2 additional properties: LastImageSet and ArtistUrl Additionally IsLoved will (more often) get an actual value Setting these properties on actual to null fixes this specific (UpdatesNowPlaying) test failure:

actual.ArtistImages = null;
actual.ArtistUrl = null;
actual.IsLoved = null;
th0mk commented 4 years ago

I've removed the integration tests from the build pipeline in #151 .

While these are useful, they're very error prone. I would still encourage people to run these tests, but they shouldn't break a build. Would like to hear other opinions on this though.