lastfm icon indicating copy to clipboard operation
lastfm copied to clipboard

Integration tests failing?

Open rikkit opened this issue 6 years ago • 7 comments

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

rikkit avatar Feb 02 '19 14:02 rikkit

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 avatar Feb 04 '19 12:02 klinge

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 avatar Feb 04 '19 12:02 klinge

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?

klinge avatar Feb 20 '19 13:02 klinge

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.

rikkit avatar Feb 20 '19 16:02 rikkit

Good thinking. Will check up on that.

klinge avatar Feb 21 '19 08:02 klinge

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;

dogguts avatar Apr 24 '19 23:04 dogguts

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.

th0mk avatar Nov 18 '19 12:11 th0mk