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.

Sign up for GitHub

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

Jump to bottom

Tests: assign the world to WorldTestBase, and a default player field #2385

Merged
merged 5 commits into from
Feb 15, 2024
Merged

Tests: assign the world to WorldTestBase, and a default player field #2385

merged 5 commits into from
Feb 15, 2024

Conversation

alwaysintreble
Copy link
Collaborator

@alwaysintreble alwaysintreble commented Oct 27, 2023 •
edited
Loading

What is this fixing or adding?

new options API is a nightmare to use from tests atm. Assigns the world to the class after it's created, and sets a player field on the base for usage because I hate typing 1 in method calls. Also some docstrings and very miniscule performance improvement

How was this tested?

how else

@ScootyPuffJr1 ScootyPuffJr1 added is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. is: refactor/cleanup Improvements to code/output readability or organizization. labels Oct 27, 2023
@black-sliver
Copy link
Member

Why not leave self.options as-is, change WorldTestBase to a generic WorldTestBase[W] and self.world: W to get self.world.options as a correctly typed solution?

@alwaysintreble
Copy link
Collaborator Author

you still wouldn't get correct typing when accessing options that way unless you added typing to self.world on your test base. The base options can get typing to at least PerGameCommonOptions after the next version, which will basically be the same behavior. I posited what the actual gain would be from writing the world instead of exclusively the options, and nothing came to mind at the time, but I suppose it would add some slight convenience in my own world, so it's probably worth it.

@alwaysintreble
Tests: assign the World to WorldTestBase and add a player field (beca…
ee47e8e 
…use I like typing self.player far more than random 1's all over the place)
@alwaysintreble alwaysintreble changed the title Tests: add test_options field and assign option results to self.options Tests: assign the world to WorldTestBase, and a default player field Oct 30, 2023
@black-sliver
Copy link
Member

As I said in the initial sdv pr. player=1 is confusing if the player variable is unused (in setup)

@alwaysintreble alwaysintreble added the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 14, 2024
@black-sliver
Copy link
Member

Is there a reason you didn't change AllSealsRequired to use self.world in test_shop_chest.py?

@alwaysintreble
Copy link
Collaborator Author

alwaysintreble commented Feb 14, 2024 •
edited
Loading

when i moved my world to the new options api i missed updating the tests to it. the other tests that are changing in this PR are accessing attributes on the world itself, not options. i already addresses all of those in my world rewrite and i'd rather not do it twice and resolve all the conflicts😓

black-sliver
black-sliver approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@black-sliver black-sliver left a comment

Choose a reason for hiding this comment

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

lgtm then, not sure if we still want a peer review?

@alwaysintreble
Copy link
Collaborator Author

i think at the very least @agilbert1412 wants this if we should

agilbert1412
agilbert1412 approved these changes Feb 15, 2024
View reviewed changes
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.

I do have a significant number of tests that could benefit from this simplified syntax, so I like it.
Code seems good.

@black-sliver black-sliver merged commit 3869a25 into ArchipelagoMW:main Feb 15, 2024
15 checks passed
@github-actions github-actions bot removed the waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. label Feb 15, 2024
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
@alwaysintreble @Jouramie
Tests: assign the world to WorldTestBase, and a default player field (A…
f0302d0 
…rchipelagoMW#2385)

* Tests: assign the World to WorldTestBase and add a player field (because I like typing self.player far more than random 1's all over the place)

* more accurate docstring for world and multiworld

* use self.player within the class
TheLX5 pushed a commit to TheLX5/Archipelago that referenced this pull request Mar 2, 2024
@alwaysintreble @TheLX5
Tests: assign the world to WorldTestBase, and a default player field (A…
d68a5ab 
…rchipelagoMW#2385)

* Tests: assign the World to WorldTestBase and add a player field (because I like typing self.player far more than random 1's all over the place)

* more accurate docstring for world and multiworld

* use self.player within the class
TheLX5 pushed a commit to TheLX5/Archipelago that referenced this pull request Mar 2, 2024
@alwaysintreble @TheLX5
Tests: assign the world to WorldTestBase, and a default player field (A…
d55132c 
…rchipelagoMW#2385)

* Tests: assign the World to WorldTestBase and add a player field (because I like typing self.player far more than random 1's all over the place)

* more accurate docstring for world and multiworld

* use self.player within the class
@alwaysintreble alwaysintreble deleted the test_options branch March 10, 2024 19:43
EmilyV99 pushed a commit to EmilyV99/Archipelago that referenced this pull request Apr 15, 2024
@alwaysintreble @EmilyV99
Tests: assign the world to WorldTestBase, and a default player field (A…
bb743a0 
…rchipelagoMW#2385)

* Tests: assign the World to WorldTestBase and add a player field (because I like typing self.player far more than random 1's all over the place)

* more accurate docstring for world and multiworld

* use self.player within the class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@black-sliver black-sliver black-sliver approved these changes

@agilbert1412 agilbert1412 agilbert1412 approved these changes

Assignees
No one assigned
Labels
is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. is: refactor/cleanup Improvements to code/output readability or organizization.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alwaysintreble @black-sliver @agilbert1412 @ScootyPuffJr1