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

Bumper Stickers: add location rules #2254

Merged
merged 12 commits into from
Oct 25, 2023

Conversation

FelicitusNeko
Copy link
Collaborator

What is this fixing or adding?

Adds location access rules for Bumper Stickers' Treasure Bumper, Bonus Booster, and All Hazards locations.

How was this tested?

Ran AP unit tests. Then generated manually a few times and reviewed the spoiler file to confirm that the behaviour is consistent with what should be happening.

If this makes graphical changes, please attach screenshots.

n/a

@alwaysintreble
Copy link
Collaborator

As mentioned in discord already, this should really be adding tests; especially when testing is one of the claims as none of the generic tests are affected by this change. You also shouldn't be setting rules in "generate basic"

@FelicitusNeko
Copy link
Collaborator Author

I'm not sure what you mean by "adding tests". As for setting rules in generate_basic, I'm guessing you mean the completion condition. That was in generate_basic before and nobody said anything, but if it needs to be moved, I can do so.

I have literally never written these before so 🤷
@black-sliver
Copy link
Member

Tests failing. So either tests bad or code bad.
If you need help figuring things out, I think talking over it in Discord is easier.

@FelicitusNeko
Copy link
Collaborator Author

Definitely bad tests. I am absolutely not practiced at writing unit tests. I'll let you know as soon as I can set some time for this.

@black-sliver
Copy link
Member

black-sliver commented Oct 7, 2023

assertAccessDependency is a rather complex test and not always the best approach. It will test that the listed locations are only ever accessible with the listed items, and verifies that everything else does not depend on those items.

In your case, you could just collect items and check that the expected latest location is accessible, but the next isn't, using

items = self.get_item_by_name("Treasure Bumper")
# some kind of loop, doing
self.collect(items[i])
self.assertTrue(self.can_reach_location(locations[i]))
self.assertFalse(self.can_reach_location(locations[i+1]))

@black-sliver
Copy link
Member

Test still failing. If you run them in pycharm, you can inspect it, or you could use subtests or a custom failure message to get a better output. i.e. assertTrue(something, "{x+1} couldn't be reached in {x}")

@FelicitusNeko
Copy link
Collaborator Author

Sorry for taking so long, but it works now.

@ThePhar ThePhar added the is: enhancement Issues requesting new features or pull requests implementing new features. label Oct 17, 2023
@black-sliver black-sliver added is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs. and removed is: enhancement Issues requesting new features or pull requests implementing new features. labels Oct 18, 2023
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.

Assuming the changed rules are actually correct, lgtm

@black-sliver black-sliver merged commit e5ca83b into ArchipelagoMW:main Oct 25, 2023
12 checks passed
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
* bumpstik: treasure/booster location rules

* bumpstik: oop missed a bit

* bumpstik: apply access rule to Hazards check

* bumpstik: move completion cond. to set_rules

* bumpstik: tests?
I have literally never written these before so 🤷

* bumpstik: oops

* bumpstik: how about this?

* bumpstik: fix some logic

* bumpstik: this almost works but not quite

* bumpstik: accurate region boundaries for BBs
since we're using rules now

* bumpstik: holy heck it works now
Jouramie pushed a commit to Jouramie/Archipelago that referenced this pull request Feb 28, 2024
* bumpstik: treasure/booster location rules

* bumpstik: oop missed a bit

* bumpstik: apply access rule to Hazards check

* bumpstik: move completion cond. to set_rules

* bumpstik: tests?
I have literally never written these before so 🤷

* bumpstik: oops

* bumpstik: how about this?

* bumpstik: fix some logic

* bumpstik: this almost works but not quite

* bumpstik: accurate region boundaries for BBs
since we're using rules now

* bumpstik: holy heck it works now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: bug/fix Issues that are reporting bugs or pull requests that are fixing bugs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants