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

The Witness: Add some unit tests #3328

Merged
merged 35 commits into from
Jul 6, 2024

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented May 18, 2024

Idk if these make any sense but yeah

Also adds a hidden option allowing me (and people who know, I guess) to turn off the "put a random symbol on Tutorial Gate Open" thing

@NewSoupVi
Copy link
Member Author

NewSoupVi commented May 18, 2024

import worlds.terraria??? lol one sec, I'll fix that

@github-actions github-actions bot added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label May 18, 2024
@Exempt-Medic Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label May 19, 2024
Copy link
Collaborator

@agilbert1412 agilbert1412 left a comment

Choose a reason for hiding this comment

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

New tests == Good!

worlds/witness/test/__init__.py Show resolved Hide resolved
NewSoupVi and others added 7 commits June 9, 2024 04:27
…FGJ'sklgösLGIKsdhJLGÖsdfjälghklDASFJghjladshfgjasdfälkjghasdöLfghasd-kjgjASDLÖGHAESKDLJGJÖsdaLGJHsadöKGjFDSLAkgjölSÄDghbASDFKGjasdLJGhjLÖSDGHLJASKDkgjldafjghjÖLADSFghäasdökgjäsadjlgkjsadkLHGsaDÖLGSADGÖLwSdlgkJLwDSFÄLHBJsaöfdkHweaFGIoeWjvlkdösmVJÄlsafdJKhvjdsJHFGLsdaövhWDsköLV-ksdFJHGVöSEKD
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Comments and questions were addressed, the tests pass and make sense for the game. The changes in __init__.py and options.py make sense as well

@NewSoupVi NewSoupVi added waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer. and removed waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jun 10, 2024
worlds/witness/options.py Outdated Show resolved Hide resolved
worlds/witness/test/__init__.py Show resolved Hide resolved
worlds/witness/test/test_ep_shuffle.py Outdated Show resolved Hide resolved
worlds/witness/test/__init__.py Show resolved Hide resolved
@black-sliver
Copy link
Member

black-sliver commented Jul 5, 2024

Collapse into one test :(

Why the sad face? :S
Besides the overhead of rolling games, I think it does make more sense to have all those checks "atomic" on a single game. (I don't expect the difference in performance to be too noticeable for most witness option combinations.)

In my mind the cut should happen where the code block stops making sense, becomes too large to follow easily or when you want a clean state/rely on modifying state (i.e. self.collect) unless it makes more sense to do atomically, i.e. soe/test/test_goal/test_fragments woiuld make no sense to run on multiple games and instead we "uncollect" Victory.

@NewSoupVi NewSoupVi requested a review from black-sliver July 5, 2024 16:16
@NewSoupVi NewSoupVi merged commit f99ee77 into ArchipelagoMW:main Jul 6, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: enhancement Issues requesting new features or pull requests implementing new features. waiting-on: core-review Issue/PR has been peer-reviewed and is ready to be merged or needs input from a core maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants