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

Adding more CI/CD tests and also fixing issue #1 #13

Merged
merged 12 commits into from
May 8, 2024
Merged

Conversation

lassefolkersen
Copy link
Owner

Here's a bunch of more tests for planets and solarsystem classes. It seemed that the solarsystem class tests, when I tried to do the save/load testing was a real good test bench to fix that issue #1 - so I think it should work better now. Could use a little more testing, although I did put in a pretty comprehensive content checker.

@JonathanLochridge
Copy link
Collaborator

From running a test, This can start a new game without crashing and can also save without crashing. But not load.

Still a massive step forward. From what I can tell. That seems like enough that we could merge it.

Copy link
Collaborator

@JonathanLochridge JonathanLochridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good,
Are the tests running automatically?
Or can we call those or perhaps add them to a workflow maybe in the future?

(More generally, setting up some workfows that are run whenever we add new code changes to ensure the essential features are still working seems like a good idea so long as it won't cost us? That might have a price, and fi so might not be worth it? not sure?

If so, then I guess we could just have a file for doing it, and make it part of the local commit procedures?

I didn't see any issues, but there were several areas where I was not familiar enough with what was being tested to be able to tell if there was something major.

@JonathanLochridge
Copy link
Collaborator

On 4th glance, I think what you did to slow down the intro sequence may have created or exposed a stutter in the animation.

As one occurs when testing this PR. Not a super big issue though.

Perhaps remove that bit from the PR, to be separate and take another crack at that specific bit separately later.

@JonathanLochridge
Copy link
Collaborator

Also, a small slightly off-topic thing I found was fascinating, apparently this repo got put in the 2020 artic code vault or something. And put on film media. Which is kind of wow. Anyways, I am excited by the work we have gotten done in the last 2 weeks.

@lassefolkersen lassefolkersen merged commit d7d864f into master May 8, 2024
3 checks passed
@lassefolkersen
Copy link
Owner Author

Ok, I merged this to master now, because I think it's a good development tool: The tests get activated automatically on every PR to master branch. Then they run them all on both a windows, a mac and an ubuntu machine. So essentially like you describe already as a part of the commit procedure:

If so, then I guess we could just have a file for doing it, and make it part of the local commit procedures?

This would be the file. You can also trigger them whenever on any branch, under 'actions', like in this image, for checking.

Screenshot 2024-05-08 at 07 45 59

Clearly they are not comprehensive enough to catch neither the stutter nor the crash on load. It's more meant as a check that nothing central breaks. Lots of moving parts. And we can expand later.

I'll make a separate issue for the stutter - good observations, I guess the problem is then that it executes differently on different computers because (now) it looks smooth to me, it didn't before).

Cool with the arctic code vault, I didn't know that.

@lassefolkersen lassefolkersen deleted the ci_cd_tests branch May 8, 2024 05:51
@JonathanLochridge
Copy link
Collaborator

Yeah, well the stutter on load is a minor error, like it wouldn't hurt much to add a check for that, but it is easily tested by running the program.

The intent is a pretty important intent.
I do think that saving and loading is enough of a core feature that ideally we should have a test to ensure their integrity that runs. I think you added tests related to that though? not 100% sure how much they cover there though.

@lassefolkersen
Copy link
Owner Author

Maybe I can take a turn with more tests later, or you can if you want --- I just used pytest and github actions. Right now I just put in for the solarsystem and planet class functions. That leaves base and firm, and maybe some. Idk. Ongoing project I guess. Just felt it would be nice with something right now and here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants