mifriis
commented
1 year ago Hi @Davieo
This looks very promising. I look forward to trying it out. Would you mind taking a few screenshots and posting them with your PR? For historic and glancing purposes it's always fun to be able to go back and see what things looked like.
It looks like some of the existing tests fail:
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
01/01/0001 00:00:00 Johnny5 has arrived in orbit around Earth
01/01/0001 00:00:00 Johnny5 has 0 tons of fuel left.
Greetings captain, what is your name?
Welcome, Captain LongLars, you have logged in on 01/01/0001 00:00:00
01/11/2023 11:33:01 A Tiny S class asteroid found with an estimated yield of 10
01/11/2023 11:33:01 It has been added to the list of prospected asteroids.
Greetings captain, what is your name?
Welcome, Captain LongLars, you have logged in on 01/01/0001 00:00:00
Greetings captain, what is your name?
Welcome, Captain LongLars, you have logged in on 01/10/2023 11:33:02
[xUnit.net 00:00:01.12] Kuiper.Tests.Unit.Services.CaptainServiceShould.LoadCaptainWhenSavesFound [FAIL]
Greetings captain, what is your name?
Welcome, Captain LongLars, you have logged in on 01/10/2023 11:33:02
[xUnit.net 00:00:01.14] Kuiper.Tests.Unit.Services.CaptainServiceShould.ReturnCaptainIfAlreadySetup [FAIL]
Failed Kuiper.Tests.Unit.Services.CaptainServiceShould.LoadCaptainWhenSavesFound [8 ms]
Error Message:
Assert.Equal() Failure
Expected: Captain { Account = Account { Balance = 100, Transactions = [...] }, LastLoggedIn = 0001-01-01T00:00:00.0000000, Name = "LongLars", Ship = Ship { Acceleration = 2.770083102493075, AccelerationGs = 0.2824698650908389, CurrentLocation = null, deltaV = 3498947.07537546, DryMass = 261, ... }, StartTime = 2023-01-10T11:33:01.9938581+00:00 }
Actual: Captain { Account = Account { Balance = 0, Transactions = [...] }, LastLoggedIn = 0001-01-01T00:00:00.0000000, Name = "LongLars", Ship = Ship { Acceleration = (throws ArgumentNullException), AccelerationGs = (throws ArgumentNullException), CurrentLocation = null, deltaV = (throws ArgumentNullException), DryMass = (throws ArgumentNullException), ... }, StartTime = 2070-02-03T00:00:00.0000000 }
Stack Trace:
at Kuiper.Tests.Unit.Services.CaptainServiceShould.LoadCaptainWhenSavesFound() in /home/runner/work/edge-of-kuiper/edge-of-kuiper/kuiper-tests/Services/CaptainServiceShould.cs:line 185
Failed Kuiper.Tests.Unit.Services.CaptainServiceShould.ReturnCaptainIfAlreadySetup [6 ms]
Error Message:
Moq.MockException :
Expected invocation on the mock exactly 1 times, but was 0 times: x => x.LookForSaves(It.IsAny<string>())An integration test around saving and loading it looks like. Saving and loading are the most frequent issue i have had during coding so it might signal that something that works in a dry run is not restored properly from the json afterwards during a load.
I see quite a few get; set; many of which where the set is added by this PR. One of the goals has been to encapsulate the logic in the classes so you can't set arbitrary values but rather are required to "set" through the constructor. This might seem a bit cumbersome, but on classes where the value of a property is calculated, it would be dangerous to allow it to also be "set". Consistency that properties are set through a constructor will ease this.
NewtonSoft has so been able to construct regardless, perhaps relying on a fallback where the parameters in the constructor matches the name of the property. What caused the need to create the setters?
Finished the ship trader. Current build only trades ships available in Data/Trader.ships. No ascii representation as that would require bigger overhaul and I think it is better suited task for module enhancement which is already an issue. Added few unit tests as school requirement, can be deleted if not suited for this project.