Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Savegame regression testing #7895

Closed
wants to merge 4 commits into from
Closed

Conversation

LordAro
Copy link
Member

@LordAro LordAro commented Jan 3, 2020

No description provided.

@LordAro LordAro force-pushed the savegame-testing branch 3 times, most recently from c0e7ff1 to 676c008 Compare January 3, 2020 18:50
@LordAro LordAro marked this pull request as ready for review January 3, 2020 19:05
@LordAro
Copy link
Member Author

LordAro commented Jan 3, 2020

Any suggestions for other savegames to add?

@nielsmh
Copy link
Contributor

nielsmh commented Jan 3, 2020

Perhaps the crazy 50k stations save?

I assume there can be problems with games that require NewGRFs, since not all are freely redistributable, and they can also be quite large. Perhaps have a feature to point to a suite of "private" additional games to test on? That can allow anyone to have their own selection of favorites, or games selected as relevant for their current work. It can also let us run extended regressions as part of CI, with a large library of games and GRFs that may not all be free for us to put in the main repository.

@LordAro
Copy link
Member Author

LordAro commented Jan 3, 2020

The 50k stations save is more for benchmarking, rather than any regression reasons - it doesn't have any other useful features!

As for GRFs - depends on the particular save. There are certainly saves (like #1131) where the crashing behaviour is independent of the GRFs, so the missing GRFs can be safely ignored. Not sure it's be particularly useful to separate them out into a separate repo.

@LordAro
Copy link
Member Author

LordAro commented Jan 3, 2020

Also I'd quite like to avoid bloating the repo more than necessary - 1MB per save is quite significant, I'm not sure I'd want to go over that

@TrueBrain TrueBrain added candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while labels Dec 14, 2020
@TrueBrain
Copy link
Member

I am sure this had the best intentions when written, and most likely talked over on IRC .. but ... what is the idea behind this PR? :D If you wouldn't mind filling me in, that would be greatly appreciated :)

I see a lot of need of having savegames to test OpenTTD against, but I am just not sure that was the idea here ;) Cheers!

@TrueBrain TrueBrain added the work: missing intention This Pull Request is missing "why" it exists label Dec 15, 2020
@TrueBrain
Copy link
Member

I have been wondering about this a bit .. I assume the idea is to test all kinds of savegames, just to see if they load at all, right?

I am not sure it belongs in the OpenTTD code-base, as every sav increases the checkout size .. but what if we setup a CI that runs this on every Pull Request? And gets the sav from some remote place (S3 bucket, ..)? That also means we can add some of those huge 100 MiB savegames etc ... also gives a nice collection of savegames to test other Pull Requests with.

Of course we can create a very simple script that downloads the savs locally, so you can test against them too.

Hell, we could integrate it in BaNaNaS if we would go bananas :D But that might be a bit weird :P

Just thinking out loud here :D

@LordAro
Copy link
Member Author

LordAro commented Dec 28, 2020

Yeah, that's not a bad alternative. As alluded to above, I was aware of the size issue, but obviously isn't really any other way when you're just working with this repo alone.

(And of course, just opening this PR has added these files to the repo >:-) )

@TrueBrain
Copy link
Member

(And of course, just opening this PR has added these files to the repo >:-) )
Git protocol v2 at least doesn't download it on clone. That is the important thing for me :P

Let's think / chat about this some more. I noticed when I wanted to test out some PRs, I really struggled finding savegames to test it with (older ones, ones with a lot of trains, lot of buses, etc). So having an archive with them would be really great to me at least.

But we need to think how we want to make that available. I have not thought about it myself really .. just that I would like that :P

Do you think with an idea of such an archive, this PR should remain open? Just as reminder that we want this? (I am just asking the question; have not formed an opinion myself :D).

@TrueBrain
Copy link
Member

Been fiddling a bit with this:

https://truebrain.github.io/OpenTTD-savegames/
https://github.com/TrueBrain/OpenTTD-savegames/

Main problem would be if we add savegames with NewGRFs and AIs. How to work with those? We currently don't have a mechanism to download BaNaNaS content when a savegame is loaded .. but this might be useful for dedicated servers too, so maybe it is time we add this. Not sure how yet .. but something I ran into.

Otherwise, this PR itself is a nice approach to test the savegames; it just needs a rebase (hihi, "just".. it needs a full rewrite because CMake).

The OpenTTD-savegames repo is far from done btw .. Python code is not pretty, many more chunks should be analysed, and every savegame should have a metadata file with at least a description. But I just wanted to show what I have been doing with this, see what other people think :)

@LordAro LordAro closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
candidate: needs considering This Pull Request needs more opinions size: large This Pull Request is large in size; review will take a while work: missing intention This Pull Request is missing "why" it exists
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants