search

inflatablefriends / lastfm

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

Feature/135 retarget test projects #136

Closed klinge closed 5 years ago

klinge commented 5 years ago

Trying to fix issue #135 (test projects depending on full .Net framework). Because of an issue in .Net Core handling of embedded files (resources) this led to more changes than I expected initially. All 101 tests are now running and passing. Please review my changes and see if you want to pull them.

klinge commented 5 years ago

Closing because not building properly

rikkit commented 5 years ago

@klinge hey, thank you so much for taking the time to do this.

I've had a quick glance at the code - looks good to me.

Like you've noticed - it's not building on AppVeyor, which it'll need to do before I can merge it. Looks like it's missing C:\projects\lastfm\src\IF.Lastfm.Core.Tests\Resources\GetTopAlbumsError.json.

Feel free to keep this PR open and add extra commits to your branch - then you'll get the use of AppVeyor checking your commits whenever you push.

klinge commented 5 years ago

OK, I'll reopen the PR. While going over the test projects I moved some of the json files to suitable directories. That is why it seems to be missing...

klinge commented 5 years ago

Bear in mind that I'm on Visual Studio Code so the newly generated csproj files for the test projects are missing all of the generated code that comes from Visual Studio. If possible please verify that things are working as expected in the full Visual Studio before merging.

rikkit commented 5 years ago

@klinge VSCode is fine, in fact that's what I use now when working on this project.

AppVeyor now builds, but the tests don't seem to be running:

// https://ci.appveyor.com/project/rikkit/lastfm/builds/20761833#L432

.\run-tests.ps1
NUnit-Console version 2.6.4.14350
Copyright (C) 2002-2012 Charlie Poole.
Copyright (C) 2002-2004 James W. Newkirk, Michael C. Two, Alexei A. Vorontsov.
Copyright (C) 2000-2002 Philip Craig.
All Rights Reserved.
Runtime Environment - 
   OS Version: Microsoft Windows NT 6.2.9200.0
  CLR Version: 2.0.50727.8793 ( Net 3.5 )
ProcessModel: Default    DomainUsage: Multiple
Execution Runtime: net-4.5
Could not load file or assembly 'IF.Lastfm.Core.Tests.dll' or one of its dependencies. The system cannot find the file specified.

Any ideas?

klinge commented 5 years ago

Being on linux I havn't tried running the powershell test command. While doing the changes I just used the Dotnet CLI and ran the command dotnet test in each of the test projects to verify that they were working. I'll have a look and see if I can understand what is happening.

klinge commented 5 years ago

@rikkit This turned out to be a real PITA :) First step was to change the paths to the test dlls (they are now in the bin\Release\netcoreapp2.1\ folder. This fixed the problem you saw. But this in turn generates an error on framework versions in nunit-console and the build enviroment in some way.

Since I cannot build the project locally (Syro project is still depending on the full .Net Framework) I'm kind of blindfolded here. I just made one small change at a time and commited to see the new appveyor build. But after a number of tries it is still not working. Would it be possible for you to have a look?

rikkit commented 5 years ago

@klinge I won't have time to check this until Christmas time really, sorry. I'd suggest a couple things

Cheers

klinge commented 5 years ago

@rikkit No, worries. It's not time-critical in any way. Just nice to make the test projects platform independent. I'll look into your suggestions. I might try to remove the dependency on nunit-console. From the command line CLI the tests are running fine with the command dotnet test. If this is possible in the appveyor build I'll go for this. Will read up on the appveyor build system.

rikkit commented 5 years ago

@klinge Ace. Thanks for the time you're putting into this, it's much appreciated

klinge commented 5 years ago

Couldn't resist looking into this. Now I have set target for the main projects back to netstandard1.1, commented out the Syro project from the solution file, set a property IsTestProject=true in the test project files (the compiler complained about this not being set) and lastly I just commented out the call to the run-tests script (appveyor docs says that test should be automatically detected and run if you do nothing special).

Build seems to run fine now including all tests executing and passing. See if you think it looks ok. If so I will add the Syro project back in to the solution.

klinge commented 5 years ago

Only thing that I can see is that there are no report on test results in the "Test tab" on Appveyor - but looking in the build log tests are running and passing..

rikkit commented 5 years ago

@klinge it seems like a custom logger is required now under .Net core? IDK https://github.com/spekt/appveyor.testlogger

klinge commented 5 years ago

Added the package you suggested to the test projects and voila! - test reporting to the Appveyor GUI works. I didn't expect it to be that easy! Starting to feel pretty finished with this now.

I really would like to remove the .resx and designer.cs file in the Resource folder under Core Tests because I think they should be redundant now - but when I do remove them I get a large number of errors when trying to build. So I'll leave them alone - for now :)

rikkit commented 5 years ago

Hi @klinge - this all LGTM. Just want to run the tests locally and I'll merge. Should have some time tonight

rikkit commented 5 years ago

Hey @klinge! I've just found time to check this out locally. I was unable to run the SQLite integration tests locally, but they seem to work on AppVeyor so I'm assuming it's an issue with my development environment. Thanks for doing this again, and sorry it took me so long to approve!