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

CrystalBallManager #332

Merged
merged 7 commits into from Dec 14, 2021
Merged

CrystalBallManager #332

merged 7 commits into from Dec 14, 2021

Conversation

fredg1
Copy link
Contributor

@fredg1 fredg1 commented Dec 12, 2021

Additional tests will obviously be required

@fredg1 fredg1 requested a review from a team as a code owner December 12, 2021 05:44
@gausie
Copy link
Contributor

gausie commented Dec 12, 2021

Yeah this looks great. Can you add one FightRequest test and some tests to isCrystalBallZone etc? In fact, maybe those functions should move to this new manager.

ckb in the forums suggested a proxy record property of turn and monster per $location, which I thought sounded like a nice idea also.

this was there because I initially thought that any request not in adventure.php
or fight.php wouldn't affect predictions

It's worth noting that while that assertion ended up not being true,
we'll most likely end up needing to use it again later.
This is because relying on KoLAdventure.lastLocationName is error prone,
because we only want to list visits to adventure.php locations,
and KoLAdventure.lastLocationName currently includes
locations such as cellar.php.

(I guess we could also try to spot them at the start of the method...)
@jaadams5
Copy link
Contributor

#318 is also waiting for some good tests of FightRequest. I'm inclined to uses @gausie' smost recent work as a model for future test. Basically capture the "right" html and then load it and test for the right things to happen.

@fredg1
Copy link
Contributor Author

fredg1 commented Dec 12, 2021

#318 is also waiting for some good tests of FightRequest. I'm inclined to uses @gausie' smost recent work as a model for future test. Basically capture the "right" html and then load it and test for the right things to happen.

Doing this for most fight requests would most likely be bloat for those test files. Could there be a dedicated location to store that html?

@gausie
Copy link
Contributor

gausie commented Dec 12, 2021 via email

@fredg1
Copy link
Contributor Author

fredg1 commented Dec 12, 2021

You can store the HTML wherever you want - check recent tests for gnomeAdv

(this sure would have been a nice use for https://kolmafia.us/threads/responsemakingutilities.26848/ ... (._. ) )
(i.e. providing a shortcut to make mock fight.php html, without having to save everything...)

@gausie
Copy link
Contributor

gausie commented Dec 12, 2021

I don't mind at all saving the whole HTML page. In fact on balance I'd probably rather we do so to catch weird edge cases. Unless you're talking about something else?

@fredg1
Copy link
Contributor Author

fredg1 commented Dec 12, 2021

no, that was pretty much it.
I was/am aiming to make it so that the builder would even allow us to cover these edge cases, by having defaults that can be overriden (for example, the content of the combat action bar, end of fight familiar actions, previous round messages...)

@jaadams5
Copy link
Contributor

no, that was pretty much it. I was/am aiming to make it so that the builder would even allow us to cover these edge cases, by having defaults that can be overriden (for example, the content of the combat action bar, end of fight familiar actions, previous round messages...)

I commented over there. My concern is a framework that is so complex that it needs its own tests.

@gausie
Copy link
Contributor

gausie commented Dec 13, 2021

Please don't build an HTML builder

@gausie
Copy link
Contributor

gausie commented Dec 13, 2021

All this needs now is a few FightRequest tests. Thanks!

@gausie
Copy link
Contributor

gausie commented Dec 14, 2021

I'll put an announcement on the forums about this pref changing and then merge this today

@gausie gausie merged commit b26f260 into kolmafia:main Dec 14, 2021
@fredg1 fredg1 deleted the CrystalBallManager branch December 14, 2021 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants