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

Test: Correct several format IDs #10633

Merged
merged 13 commits into from
Oct 29, 2024
Merged

Test: Correct several format IDs #10633

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
8450871
RandBat Test: Correct `vType` for gen9 bss factory
larry-the-table-guy Oct 26, 2024
61c8bad
RandBat Test: Fix format id logic for `Uber` sets
larry-the-table-guy Oct 26, 2024
1d933ff
RandBat Test: Use existent AG format
larry-the-table-guy Oct 26, 2024
7078ff5
Test sim/misc: Ensure that format exists
larry-the-table-guy Oct 26, 2024
8a379a9
RandBat Test: Use `bssregh` instead of `bssfactory` as `vType`
larry-the-table-guy Oct 26, 2024
c049d31
Sim/TeamValidator: Also identify rules tagged with '*'
larry-the-table-guy Oct 26, 2024
208e834
RandBat Test: Remove `gen9caprandombattle` tests
larry-the-table-guy Oct 26, 2024
dee503b
Sim/TeamValidator: Check that format exists and is a Format
larry-the-table-guy Oct 26, 2024
b26ff73
Test/TeamValidator: Bump PH and AG gen numbers
larry-the-table-guy Oct 26, 2024
e9670c8
Test/common: Check `effectType` in `getFormat`
larry-the-table-guy Oct 26, 2024
3497ecc
Test: Simplify checks for valid Format
larry-the-table-guy Oct 27, 2024
3c44bec
Test/common: Load `dex`'s data
larry-the-table-guy Oct 28, 2024
b65d38c
RandBat Test: Restore `gen9` CAP Rand with adjusted format name
larry-the-table-guy Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions test/random-battles/all-gens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,8 +304,16 @@ describe('Battle Factory and BSS Factory data should be valid (slow)', () => {

for (const type in setsJSON) {
const typeTable = filename.includes('bss-factory-sets') ? setsJSON : setsJSON[type];
const vType = filename.includes('bss-factory-sets') ? `battle${genNum === 8 ? 'stadium' : 'spot'}singles` :
type === 'Mono' ? 'monotype' : type.toLowerCase();
let vType;
if (filename.includes('bss-factory-sets')) {
vType = `battle${genNum === 8 ? 'stadium' : 'spot'}singles`;
} else if (type === 'Mono') {
vType = 'monotype';
} else if (type === 'Uber') {
vType = 'ubers';
} else {
vType = type.toLowerCase();
}
for (const species in typeTable) {
const speciesData = typeTable[species];
for (const set of speciesData.sets) {
Expand Down Expand Up @@ -372,7 +380,7 @@ describe('[Gen 9] BSS Factory data should be valid (slow)', () => {
const genNum = 9;

for (const speciesid in setsJSON) {
const vType = 'battlestadiumsingles';
const vType = 'bssfactory';
Copy link
Member

Choose a reason for hiding this comment

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

I believe this should be bssregh, since it's validating off of the base battle stadium format legality. Obviously a fix of checking if gen9bssregh exists would make sure people update this since there's no easy way to automate this apart from trying to assign an alias to the BSS format that's active in aliases.ts, but if you can figure something else out I'm all ears.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All I could come up with so far is validating that the formats exist. The alias is an interesting idea, kind of like how 'base' is an alias for the current gen's dex.

let totalWeight = 0;
for (const set of setsJSON[speciesid].sets) {
totalWeight += set.weight;
Expand Down
2 changes: 1 addition & 1 deletion test/sim/misc/megaevolution.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ describe('Mega Evolution', function () {
assertLegalButCantMega('gen9nationaldexag@@@-ndag');

// don't add it where unnecessary
assert.false(Dex.formats.getRuleTable(Dex.formats.get('gen5anythinggoes')).has('megarayquazaclause'));
assert.false(Dex.formats.getRuleTable(Dex.formats.get('gen4anythinggoes')).has('megarayquazaclause'));
Copy link
Contributor

@Slayer95 Slayer95 Oct 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Just like Gen 5 AG, Gen 4 AG is not guaranteed to exist in the future.

Can you fix this in a more reliable way?

Even just assert(Dex.formats.get('gen4anythinggoes').exists) can do the trick, so that this test gets properly updated if/when Gen 4 AG is deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, made the change suggested in the later comment.

The code would now get this error message:

  2690 passing (5m)
  90 pending
  1 failing

  1) Mega Evolution
       Mega Rayquaza
         should implicitly add the Mega Rayquaza Clause when banned:
     Error: Unidentified format: gen5anythinggoes
      at TestTools.getFormat (test/common.js:47:30)
      at Context.<anonymous> (test/sim/misc/megaevolution.js:166:26)
      at processImmediate (node:internal/timers:476:21)

The other two instances of formats.get don't seem like they should / need to be changed.

$ grep -rn "formats.get(" ./test/
./test/random-battles/all-gens.js:31:	const format = Dex.formats.get(`${formatID}@@@Max Move Count = ${count}`);
./test/sim/team-validator/basic.js:11:	Dex.formats.getRuleTable(Dex.formats.get(format));
./test/common.js:46:	const format = Dex.formats.get(options.formatid);
./test/common.js:79:	format = Dex.formats.get(formatName);

Copy link
Contributor

Choose a reason for hiding this comment

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

Something similar for helpers that wrap TeamValidator.get(), probably?

pokemon-showdown/test/random-battles/tools.js

Lines 179 to 180 in 7078ff5

function validateLearnset(move, set, tier, mod = 'gen8') {
const validator = TeamValidator.get(`${mod}${tier}`);

if (!validator.format.exists) ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about checking format.exists in the TeamValidator constructor?
My understanding is that if you reached this path in the Format constructor, then you get an empty ruletable, which probably isn't interesting for team validation.

I suppose someone could construct a populated Format and add an exists: false property, but I don't see why they would knowingly do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Except that I have just remembered that format.exists is often a useless flag. format.effectType == 'Format' should be checked instead, to distinguish actual formats from rulesets. Other commits might need adjustments accordingly.

Copy link
Contributor

@Slayer95 Slayer95 Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Condition is PS's equivalent to JavaScript's Object. In JS, everything is an Object. In PS, everything is a Condition (formerly called Effect). Given the ambiguity between formats, rulesets, and other stuff, Condition is indeed fitting. The Condition semantics are further elaborated by the fact that Format and Ruleset may and often include event handlers.

Of course, this is likely surprising for many, but it's a direct consequence of the design or lack thereof of PS format system (not favoring either side, I can see the benefits of the current system, though certainly it can be improved).

Also, these lines at dex-formats.ts don't help, which are certainly incorrect:

type FormatEffectType = 'Format' | 'Ruleset' | 'Rule' | 'ValidatorRule';
this.effectType = Utils.getString(data.effectType) as FormatEffectType || 'Format';

Having said all that, I did forget these facts at the time of reading your quiz, because I have actually been a very inactive contributor. But, hey, that was the reason why I remembered and suggested that we should just check that effectType is 'Format'. It's indeed enough ;D

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Since you are interested in the findEventHandlers situation, that link should be very useful to you.

Copy link
Contributor Author

@larry-the-table-guy larry-the-table-guy Oct 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So I looked and it seems that the effectType convention for invalid Formats dates back to the start of the repo and the introduction of getFormat. Ah well. I just feel bad for the next person who has to infer why a Format sometimes isn't a 'Format'.

I ended up going with common.getFormat because that would have caught the expired CAP case, and gen9baby isn't a mod so common.mod would crash.

Original 
for (const format of Object.keys(formatInfo)) {
        const filename = formatInfo[format].filename;
        const setsJSON = require(`../../dist/data/random-battles/${filename}.json`);
        const mod = filename.split('/')[0];
        const genNum = parseInt(mod[3]);
        const rounds = 100;
        const dex = Dex.forFormat(format);
        console.log(format, genNum, dex.gen);

=>

gen2randombattle 2 2
gen3randombattle 3 3
gen4randombattle 4 4
gen5randombattle 5 5
gen6randombattle 6 6
gen7randombattle 7 7
gen9randombattle 9 9
gen9randomdoublesbattle 9 9
gen9babyrandombattle 9 9
v2 (to demonstrate footgun in getFormat) 
for (const format of Object.keys(formatInfo)) {
        const filename = formatInfo[format].filename;
        const setsJSON = require(`../../dist/data/random-battles/${filename}.json`);
        const dex = common.mod(common.getFormat(format).mod).dex; // .getFormat verifies format exists
        console.log(format, dex.gen);
        const rounds = 100;

=>

gen2randombattle 9
gen3randombattle 9
gen4randombattle 9
gen5randombattle 9
gen6randombattle 9
gen7randombattle 9
gen9randombattle 9
gen9randomdoublesbattle 9
gen9babyrandombattle 9

Fails in a very non-obvious fashion.

v3 
for (const format of Object.keys(formatInfo)) {
        const filename = formatInfo[format].filename;
        const setsJSON = require(`../../dist/data/random-battles/${filename}.json`);
        const dex = common.mod(common.getFormat({formatid: format}).mod).dex; // verifies format exists
        const genNum = dex.gen;
        console.log(format, genNum);
        const rounds = 100;

=>

gen2randombattle 2
gen3randombattle 3
gen4randombattle 4
gen5randombattle 5
gen6randombattle 6
gen7randombattle 7
gen9randombattle 9
gen9randomdoublesbattle 9
gen9babyrandombattle 9

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to use v3 if you prefer it. But it looks like common.mod should also be calling loadData() itself. I am not sure what the state of your other PRs is, so maybe that can be done later, to avoid conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

While we are still doing archaeology, I tracked the gen loading issue to 3716f36.

This basically means you don't need to do Tools.includeData() or anything like that anymore

Is TypeScript full of void promises??!

});
});
});
Loading