Test: Correct several format IDs #10633
Conversation
`gen5anythinggoes` is not defined. gen4 is the only AG format before gen6 (mega ray is the thing being tested)
test/sim/misc/megaevolution.js
Outdated
| @@ -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')); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
Something similar for helpers that wrap TeamValidator.get(), probably?
pokemon-showdown/test/random-battles/tools.js
Lines 179 to 180 in 7078ff5
if (!validator.format.exists) ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Right. Since you are interested in the findEventHandlers situation, that link should be very useful to you.
There was a problem hiding this comment.
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 9Fails 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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??!
|
Best forwards-safe fix would be to migrate to |
Format is
I think team-validator.ts:469,618,etc should also guard against tag marker
Can this test or this part of the test be skipped? |
test/random-battles/all-gens.js
Outdated
| @@ -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'; | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Previously resulted in formats being erroneously constructed
The format was already removed.
I'll take this to mean it(`should enforce the 3 perfect IV minimum on legendaries with Gen 6+ origin`, function () {
const team = [
{species: 'xerneas', ability: 'fairyaura', moves: ['snore'], ivs: {hp: 0, atk: 0, def: 0, spa: 0}, evs: {hp: 1}},
];
assert.false.legalTeam(team, 'gen8anythinggoes');
assert.legalTeam(team, 'gen9purehackmons');
});
👍
Looks like the format was removed a few commits ago, so I removed the test case. |
The format is still playable by challenging a user to |
To ensure that the gen field is initialized
|
Looks good, thanks! |
Found some instances where formats were being created w/ exists=false.
I made changes for the cases where a correct ID seemed obvious.
Those were:
gen6uber->gen6ubersgen7uber->gen7ubersgen9battlestadiumsingles->gen9bssfactory(test label mentions factory)gen5anythinggoes->gen4anythinggoes(gen5 AG DNE)Code for finding these
Other instances of Formats where
exists == false, may not be bugs.Empty format names, seems intentional
'gen8purehackmons'
'pokemontagrestrictedlegendary'
'gen9caprandombattle'