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: add a test for worlds to not modify the itempool after create_items #1460

Merged
merged 18 commits into from
Jan 14, 2024
Merged

Tests: add a test for worlds to not modify the itempool after create_items #1460

merged 18 commits into from
Jan 14, 2024

Conversation

alwaysintreble
Copy link
Collaborator

@alwaysintreble alwaysintreble commented Feb 16, 2023 •
edited
Loading

What is this fixing or adding?

Tests

Known worlds left to fix if this is to be implemented as a test:

  • ArchipIDLE modified the itempool during generate_basic
  • Blasphemous modified the itempool during generate_basic
  • ChecksFinder modified the itempool during generate_basic
  • Clique modified the itempool during generate_basic
  • Dark Souls III modified the itempool during generate_basic
  • Donkey Kong Country 3 modified the itempool during generate_basic
  • Factorio modified the itempool during generate_basic
  • Hylics 2 modified the itempool during generate_basic
  • Kingdom Hearts 2 modified the itempool during generate_basic
  • Links Awakening DX modified the itempool during pre_fill
  • Ocarina of Time modified the itempool during pre_fill
  • Pokemon Red and Blue modified the itempool during pre_fill
  • Raft modified the itempool during generate_basic
  • Starcraft 2 Wings of Liberty modified the itempool during generate_basic
  • Super Metroid modified the itempool during generate_basic
  • Super Mario 64 modified the itempool during generate_basic
  • Super Mario World modified the itempool during generate_basic
  • SMZ3 modified the itempool during generate_basic
  • Slay the Spire modified the itempool during generate_basic
  • VVVVVV modified the itempool during generate_basic
  • Wargroove modified the itempool during generate_basic

How was this tested?

Tests

If this makes graphical changes, please attach screenshots.

N/A

@Jarno458
Copy link
Collaborator

Plz also update the description of create_items in worlds/AutoWorld.py

Jarno458 added a commit to Jarno458/Archipelago that referenced this pull request Feb 27, 2023
@Jarno458
Timespinner: Refactorings + fix for ArchipelagoMW#1460
ebe8e38
@Jarno458 Jarno458 mentioned this pull request Feb 27, 2023
Merged
Berserker66 pushed a commit that referenced this pull request Mar 4, 2023
@Jarno458
Timespinner: Refactorings + fix for #1460 (#1484)
a4b6111
@ThePhar ThePhar mentioned this pull request Apr 5, 2023
Merged
@ThePhar
Copy link
Member

ThePhar commented Apr 5, 2023

Just for awareness for other developers:

@LegendaryLinux @TRPG0 @jonloveslegos @Br00ty @Berserker66 @PoryGone @JaredWeakStrike @zig-for @Alchav @SunnyBat @TheCondor07 @lordlou @N00byKing @KonoTyran @espeon65536 @FlySniper

@Br00ty
Copy link
Contributor

Br00ty commented Apr 5, 2023

@Marechal-L tagging you in this too in case you can get to this before I can

@N00byKing N00byKing mentioned this pull request Apr 6, 2023
Merged
This was referenced Apr 29, 2023
Merged
Merged
el-u
el-u reviewed Apr 29, 2023
View reviewed changes
@el-u
Copy link
Collaborator

el-u commented Apr 29, 2023

In the docstring of create_items, the wording "should not" can be replaced by "must not".

@ThePhar ThePhar added is: documentation Improvements or additions to documentation. is: refactor/cleanup Improvements to code/output readability or organizization. labels May 31, 2023
@Zunawe Zunawe mentioned this pull request Jun 11, 2023
Merged
kindasneaki pushed a commit to kindasneaki/Archipelago that referenced this pull request Jun 28, 2023
@Jarno458 @kindasneaki
Timespinner: Refactorings + fix for ArchipelagoMW#1460 (ArchipelagoMW…
b2868db 
…#1484)
@alwaysintreble alwaysintreble changed the title Tests: add a test for worlds to only modify the itempool in create_items Tests: add a test for worlds to not modify the itempool after create_items Jul 4, 2023
@ThePhar ThePhar added the is: maintenance Regular updates to requirements and utilities that do not fix bugs or change/add features. label Jul 4, 2023
@KonoTyran KonoTyran mentioned this pull request Jul 4, 2023
Merged
Ziktofel added a commit to Ziktofel/Archipelago that referenced this pull request Jul 4, 2023
@Ziktofel
Fix SC2 for ArchipelagoMW#1460
3a0b22f
@Ziktofel Ziktofel mentioned this pull request Jul 4, 2023
Merged
@el-u el-u mentioned this pull request Jul 29, 2023
Merged
@el-u
Copy link
Collaborator

el-u commented Aug 1, 2023 •
edited
Loading

During set_rules, ALttP might try to modify the itempool in order to be able to place an extra TR smallkey.

@el-u
Copy link
Collaborator

el-u commented Aug 1, 2023

FF1 does all of its item creation in generate_basic.

@black-sliver
Copy link
Member

What's the state of this? Should we merge with the list of shame or try to fix the simple ones (i.e. Checksfinder)?

@el-u
Copy link
Collaborator

el-u commented Oct 7, 2023

What's the state of this? Should we merge with the list of shame or try to fix the simple ones (i.e. Checksfinder)?

@Berserker66
Copy link
Member

The smallkey thing was introduced by @espeon65536 , so that'd be first idea point.

@el-u
Copy link
Collaborator

el-u commented Oct 13, 2023

Current status:

  • FF1 has been resolved in FFR: create items in the correct place #2291
  • LADX, OoT, and SMZ3 fail this test, all of them during pre_fill (happens always)
  • ALttP does not fail this test, but nevertheless can modify the itempool during set_rules (could happen sometimes, depending on settings and ER randomness)

@black-sliver
Copy link
Member

  • ALttP does not fail this test, but nevertheless can modify the itempool during set_rules (could happen sometimes, depending on settings and ER randomness)

Option1: we create an issue and ignore it not being tested here
Option2: we run the create_items test as part of TestBase so it runs with all available options =

@BootsinSoots BootsinSoots mentioned this pull request Oct 13, 2023
Merged
@alwaysintreble alwaysintreble mentioned this pull request Oct 14, 2023
Open
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
@Jarno458 @FlySniper
Timespinner: Refactorings + fix for ArchipelagoMW#1460 (ArchipelagoMW…
0b7365f 
…#1484)
@ReverM
Copy link
Contributor

ReverM commented Dec 6, 2023

Do we know if some of the newly implemented games do that?

@N00byKing N00byKing mentioned this pull request Dec 7, 2023
Merged
@alwaysintreble
Merge branch 'main' into test_item_creation
103e052 
# Conflicts:
#	test/general/test_items.py
@alwaysintreble
Copy link
Collaborator Author

i believe #2311 is the only thing this is currently "waiting" on? i don't think it's worth holding off merging this any longer since we have the open issue for it even if issues aren't followed very closely

kl3cks7r pushed a commit to kl3cks7r/Archipelago that referenced this pull request Dec 15, 2023
@FlySniper
Wargroove (ArchipelagoMW#1982)
837172f 
## What is this fixing or adding?

Adjusted some map terrain. Made Ambushed in the Middle's HQ more exposed. Made Deep Thicket's AI spawn extra units. Adjusted some terrain in Rebel Village.
Moved item creation from generate_basic to create_items for (ArchipelagoMW#1460)
black-sliver
black-sliver approved these changes Jan 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.

Thanks!

@black-sliver black-sliver merged commit cfd7581 into ArchipelagoMW:main Jan 14, 2024
@alwaysintreble alwaysintreble deleted the test_item_creation branch January 27, 2024 04:08
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
@FlySniper @Jouramie
Wargroove (ArchipelagoMW#1982)
715b3fe 
## What is this fixing or adding?

Adjusted some map terrain. Made Ambushed in the Middle's HQ more exposed. Made Deep Thicket's AI spawn extra units. Adjusted some terrain in Rebel Village.
Moved item creation from generate_basic to create_items for (ArchipelagoMW#1460)
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
@alwaysintreble @black-sliver
@Berserker66 @Jouramie
Tests: add a test for worlds to not modify the itempool after `create…
129ece5 
…_items` (ArchipelagoMW#1460)

* Tests: add a test for worlds to only modify the itempool in `create_items`

* extend test multiworld setup instead of a new function

* cleanup the test a bit

* put more strict wording in `create_items` docstring

* list of shame

* Don't call `set_rules` before testing

* remove ChecksFinder from the list of shame

---------

Co-authored-by: black-sliver <59490463+black-sliver@users.noreply.github.com>
Co-authored-by: Fabian Dill <Berserker66@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@el-u el-u el-u left review comments

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

Assignees
No one assigned
Labels
is: documentation Improvements or additions to documentation. 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.
8 participants
@alwaysintreble @Jarno458 @ThePhar @Br00ty @el-u @black-sliver @Berserker66 @ReverM