Fixed clear time and mission completion tracking #68
Conversation
today I learned what csharpier is, need to get that extension later
| DbQuest? oldQuestData = await this.questRepository | ||
| .GetQuests(this.DeviceAccountId) | ||
| .SingleOrDefaultAsync(x => x.QuestId == session.DungeonId); | ||
|
|
||
| bool isFirstClear = oldQuestData is null || oldQuestData.PlayCount == 0; | ||
| bool? oldMissionClear1 = oldQuestData?.IsMissionClear1; | ||
| bool? oldMissionClear2 = oldQuestData?.IsMissionClear2; | ||
| bool? oldMissionClear3 = oldQuestData?.IsMissionClear3; |
There was a problem hiding this comment.
I think DungeonStartController will create the quest in the db, so you could use SingleAsync and ditch the null checking
There was a problem hiding this comment.
Found out that DungeonRecordTest has a bunch of null fields passed into request so ended up keeping it to prevent them from throwing errors
| missions_clear_set = Enumerable | ||
| .Range(1, 3) | ||
| missions_clear_set = clearedMissions | ||
| .Where((x, index) => (index is > 0 and < 4) && x) |
There was a problem hiding this comment.
is limiting the index necessary here?
There was a problem hiding this comment.
I think so, otherwise it'll include first clear bonus and all mission cleared bonus into the list as well which might cause problems?
There was a problem hiding this comment.
oh right, I misunderstood. Maybe it would be better to keep those variables out of the array in that case? seeing as they're used for different things
There was a problem hiding this comment.
Oh I'll do that then ye, I don't think it should mess with the logic of all missions cleared either since it's impossible for a first clear mission to be the last and only mission necessary to fulfill it
| bool[] clearedMissions = new bool[5] | ||
| { | ||
| isFirstClear, | ||
| oldMissionClear1 != newQuestData.IsMissionClear1 && newQuestData.IsMissionClear1, |
There was a problem hiding this comment.
would it be clearer to write
| oldMissionClear1 != newQuestData.IsMissionClear1 && newQuestData.IsMissionClear1, | |
| newQuestData.IsMissionClear1 && !oldMissionClear1, |
There was a problem hiding this comment.
Oh yeah will change those tyty
Handle null exceptions for existing test cases
Distinguishing between certain missions
DungeonRecordController tests and tweaks
| this.mockQuestRepository | ||
| .Setup(x => x.GetQuests(DeviceAccountId)) | ||
| .Returns(new List<DbQuest>() | ||
| { | ||
| new() | ||
| { | ||
| DeviceAccountId = DeviceAccountId, | ||
| QuestId = questId, | ||
| BestClearTime = clearTime | ||
| } | ||
| }.AsQueryable().BuildMock()); |
There was a problem hiding this comment.
You already have a setup for almost this exact same thing in the constructor
this.mockQuestRepository
.Setup(x => x.GetQuests(DeviceAccountId))
.Returns(new List<DbQuest>()
{
new() { DeviceAccountId = DeviceAccountId, QuestId = questId }
}.AsQueryable().BuildMock());It's personal preference more than anything but I like to avoid putting setups in the constructor and keep them in the test methods, so that it's clear reading the method what I'm testing. Unless it's boilerplate stuff and not really relevant (I would say FinishDungeon falls under that umbrella). It's up to you, but it would probably be a good idea to remove the duplicate ones at least.
| // Tests that when previous missions have been completed that the right amount of wyrmite is | ||
| // given as well as their corresponding mission numbers in missions_clear_set | ||
| [Fact] | ||
| public async Task VariousPreviousMissionClearStatusesGiveCorrectMissions() |
There was a problem hiding this comment.
When you're doing multiple test cases and you want to assert similarity on all of them, you can do a [Theory] test. This allows you to pass in parameters to the test function and repeat the same code with different values.
Outside of that, [Fact] unit tests should generally only contain one test case in an arrange -> act -> assert pattern.
Have a look at TheoryData in particular -- you may be able to have a list of values like GetQuests quest, CompleteQuests quest and expected missions in the response and then just repeat the same code of setup repository, call method, check response for each case
| .Setup( | ||
| x => x.AddMaterials(DeviceAccountId, It.IsAny<List<Materials>>(), It.IsAny<int>()) | ||
| ) | ||
| .Returns(Task.FromResult(0)); |
There was a problem hiding this comment.
I would do .Returns(Task.CompletedTask) for plain async Task methods
| QuestData = new QuestData( | ||
| Id: questId, | ||
| QuestPlayModeType: QuestPlayModeTypes.None, | ||
| LimitedElementalType: 0, | ||
| LimitedElementalType2: 0, | ||
| LimitedWeaponTypePatternId: 0, | ||
| PayStaminaSingle: 0, | ||
| PayStaminaMulti: 0, | ||
| DungeonType: DungeonTypes.None, | ||
| Scene01: "", | ||
| AreaName01: "", | ||
| Scene02: "", | ||
| AreaName02: "", | ||
| Scene03: "", | ||
| AreaName03: "", | ||
| Scene04: "", | ||
| AreaName04: "", | ||
| Scene05: "", | ||
| AreaName05: "", | ||
| Scene06: "", | ||
| AreaName06: "" |
There was a problem hiding this comment.
May be more concise to do
| QuestData = new QuestData( | |
| Id: questId, | |
| QuestPlayModeType: QuestPlayModeTypes.None, | |
| LimitedElementalType: 0, | |
| LimitedElementalType2: 0, | |
| LimitedWeaponTypePatternId: 0, | |
| PayStaminaSingle: 0, | |
| PayStaminaMulti: 0, | |
| DungeonType: DungeonTypes.None, | |
| Scene01: "", | |
| AreaName01: "", | |
| Scene02: "", | |
| AreaName02: "", | |
| Scene03: "", | |
| AreaName03: "", | |
| Scene04: "", | |
| AreaName04: "", | |
| Scene05: "", | |
| AreaName05: "", | |
| Scene06: "", | |
| AreaName06: "" | |
| QuestData = MasterAsset.QuestData.Get(100010101)` |
which is the first campaign quest. This is if you're not particularly interested in asserting what the quest is
| Party = new List<PartySettingList>(), | ||
| QuestData = new QuestData( | ||
| Id: questId, |
There was a problem hiding this comment.
Could you also add a test to make sure that the party and quest id values appear in the response as expected? Search for quest_party_setting_list and quest_id
No description provided.