Implement tests for vendors

[Yggdrasill]

Hello,

First off I would like to apologise for the low quality of my previous PR, and the overall mess it was to review it. I promise that my PRs will be of higher quality in the future, hopefully starting with this one.

As part of my effort to use StaticVector for vendors, I have implemented a range of tests for the vendors to ensure that the item generation is identical in the relevant ways (item types, amount of items. premium item qlvl etc.). That effort is now mostly complete and do pass these tests, but I figure that separating these into two different PRs makes sense. This will also ensure that any modification to vendors in the future need to pass these tests, and that one can determine correctness at a glance.

Regards,
yggdrasill

[Yggdrasill]

So the current build failures appear to be due to a sourceware.org timeout, and not anything in the actual code in this PR.

[AJenbo]

Personally I generally prefer test that do not have a randomized outcome since they can sporadically fail making it hard to both bisect and debug and they can miss errors that then randomly block another PR for unknown reasons.

It also means you do not end up simply duplicating the games logic in the assertion but test for a very specific outcome that is mostly free of logic.

[Yggdrasill]

Personally I generally prefer test that do not have a randomized outcome since they can sporadically fail making it hard to both bisect and debug and they can miss errors that then randomly block another PR for unknown reasons.

I can set a static seed, and I understand your position completely on that matter.

It also means you do not end up simply duplicating the games logic in the assertion but test for a very specific outcome that is mostly free of logic.

This will require some explanation on my part. The idea here was that I was intending to change some more vendor code in the future. The tests are to ensure that the same qlvls and item types are generated, but not necessarily the same items exactly. I believe this can be done without morphing because the actual item generation code is in SpawnOnePremium() and not SpawnPremium() itself. Specifically, this code:

	while (PremiumItemLevel < lvl) {
		PremiumItemLevel++;
		if (gbIsHellfire) {
			// Discard first 3 items and shift next 10
			std::move(&PremiumItems[3], &PremiumItems[12] + 1, &PremiumItems[0]);
			SpawnOnePremium(PremiumItems[10], PremiumItemLevel + itemLevelAddHf[10], player);
			PremiumItems[11] = PremiumItems[13];
			SpawnOnePremium(PremiumItems[12], PremiumItemLevel + itemLevelAddHf[12], player);
			PremiumItems[13] = PremiumItems[14];
			SpawnOnePremium(PremiumItems[14], PremiumItemLevel + itemLevelAddHf[14], player);
		} else {
			// Discard first 2 items and shift next 3
			std::move(&PremiumItems[2], &PremiumItems[4] + 1, &PremiumItems[0]);
			SpawnOnePremium(PremiumItems[3], PremiumItemLevel + itemLevelAdd[3], player);
			PremiumItems[4] = PremiumItems[5];
			SpawnOnePremium(PremiumItems[5], PremiumItemLevel + itemLevelAdd[5], player);
		}
	}

Iterates in the SpawnOnePremium() loop about ~300 times when starting a game with a level 36 character, because it has to generate 105 new items (Hellfire). My idea was to generate only 15 level-appropriate items with the correct qlvls, and handle level ups elsewhere. For sure not the most useful optimisation ever, since it does just happen once at the start of a new game, but it is an easy fix nonetheless.

I can forego that idea, and I can change the test to only test for a specific outcome. In that case, if I did make the change, the test would fail because less items are generated and they wouldn't be the same (though the item generation logic would be). That's not a difficult thing to fix, simply change the outcome tested for, but it makes it harder to prove that the change behaves correctly.

[AJenbo]

Wouldn't that change mean that items in the store morph in save games between the two versions?

[AJenbo]

This is because they can generate Hellfire specific affixes or spell books (and indeed, the crash does test if you remove these guards).

Could you let me know where it crashes, it's curious since we have item generation tests that generate hellfire items without crashes. Maybe there is some code we just need to not invoke in headless.

[Yggdrasill]

Wouldn't that change mean that items in the store morph in save games between the two versions?

I don't believe so, because the call chain looks as follows when loading a game:

RecreateItem()
RecreateTownItem()
RecreatePremiumItem()

When SpawnOnePremium() attempts to generate items, it simply advances the seed repeatedly until it manages to generate an item that passes the checks. In RecreatePremiumItem() the actual item generation code looks identical to the item generation code in SpawnOnePremium(), except with a fixed seed which has been loaded from the save.

	SetRndSeed(iseed);
	_item_indexes itype = RndPremiumItem(player, plvl / 4, plvl);
	GetItemAttrs(item, itype, plvl);
	GetItemBonus(player, item, plvl / 2, plvl, true, !gbIsHellfire);

Since the seed is saved for the item, it should create the same item when loading the game. Of course I will make sure things do not morph when I get around to implementing it, and wouldn't open a PR if I find that they do.

Could you let me know where it crashes, it's curious since we have item generation tests that generate hellfire items without crashes. Maybe there is some code we just need to not invoke in headless.

Okay, well, I was just mistaken to be honest. I can't reproduce it on the branch that I actually pushed and so I must've done something locally that caused it. Sorry about the confusion, and I'll remove the rest of them now.

Never mind.

[Yggdrasill]

Hi,

So I've done some changes to this now that I've found the time. All the premium item generation now uses static test cases, and the function test_premium_qlvl is gone. The array for expected qlvls is initialised as constexpr in the scope where it's relevant. Additionally, some things have been moved into the fixture class' SetUp() function.

Besides that, I've also split up PremiumQlvl and PremiumQlvlHf into more test cases, since I think their scope justifies it. I hope this test suite is better now. I know it's a lot of code to review, but hopefully it won't be a big pain.

Regards,
yggdrasill

[AJenbo]

Nice work, was to bad to review it either.

... oh but feel free to amend commits 27 of them doesn't seem to benefit the logic :)