Bug: `Ability regex does not overmatch` test is silently ignored

[Bing-su]

Description

The Ability regex does not overmatch unit test in test/unittests/regex_test.ts is silently ignored

Additional Details / Steps to Reproduce

You can reproduce the issue by running the test with an intentional failure.

  logDefsToTest.forEach((type) => {
    it(type, () => {
      const baseFunc = (params?: RegexUtilParams) => buildRegex(type, params);
      const Helper = new RegexTestUtil(type, examples[type], baseFunc, true);
      Helper.run();

      if (type === 'Ability')
        it('Ability regex does not overmatch', () => {
          // Tests a bug where a :1E: later in the line would be caught by overzealous
          // matchers on source names.
          // TODO: Preserved for historical reasons - maybe remove?
          const testLine =
            '[20:29:39.392] ActionEffect 15:107B9AC8:Tako Yaki:07:Attack:40017D58:Daxio:710003:DC0000:1E:50000:1C:1B60000:550003:2CA000:0:0:0:0:0:0:0:0:8207:24837:7230:7230:0:1000:527.5806:-362.7833:-19.61513:2.898741:8998:8998:10000:10000:0:1000:528.6487:-365.8656:-22.08109:-0.3377206:000AD7BF:0:1';
          Helper.captureTest((params?: RegexUtilParams) => Regexes.ability(params), [testLine]);
          const abilityHallowed = Regexes.ability({ id: '1E' });
          assert.isNull(testLine.match(abilityHallowed));
          assert.ok(false);  //
        });
    });
  });

Additionally, while modifying the test to run, I noticed that the following test failed.

          Helper.captureTest((params?: RegexUtilParams) => Regexes.ability(params), [testLine]);

Cactbot Module

Dev Tools / Internals

Configuration Info

commit: 11da0d0

Log & Screenshots

No response

Confirmation

• I reviewed Common Issues and Workarounds and verified that my settings are correct.
• I used the search function to confirm this issue doesn't already exist.

[valarnin]

Nested it functions/tests are ignored by mocha. The "sub-test" should probably not be wrapped in an it call, or should be moved to a separate top-level test, i.e. one of the two options below. I'm not sure if other test cases also have this problem.

I'm also not sure what part of captureTest you're expecting to fail here.

---

  logDefsToTest.forEach((type) => {
    it(type, () => {
      const baseFunc = (params?: RegexUtilParams) => buildRegex(type, params);
      const Helper = new RegexTestUtil(type, examples[type], baseFunc, true);
      Helper.run();

      if (type === 'Ability') {
        // Tests a bug where a :1E: later in the line would be caught by overzealous
        // matchers on source names.
        // TODO: Preserved for historical reasons - maybe remove?
        const testLine =
          '[20:29:39.392] ActionEffect 15:107B9AC8:Tako Yaki:07:Attack:40017D58:Daxio:710003:DC0000:1E:50000:1C:1B60000:550003:2CA000:0:0:0:0:0:0:0:0:8207:24837:7230:7230:0:1000:527.5806:-362.7833:-19.61513:2.898741:8998:8998:10000:10000:0:1000:528.6487:-365.8656:-22.08109:-0.3377206:000AD7BF:0:1';
        Helper.captureTest((params?: RegexUtilParams) => Regexes.ability(params), [testLine]);
        const abilityHallowed = Regexes.ability({ id: '1E' });
        assert.isNull(testLine.match(abilityHallowed), 'Ability regex does not overmatch');
      }
    });
  });

  logDefsToTest.forEach((type) => {
    it(type, () => {
      const baseFunc = (params?: RegexUtilParams) => buildRegex(type, params);
      const Helper = new RegexTestUtil(type, examples[type], baseFunc, true);
      Helper.run();
    });
  });

  it('Ability regex does not overmatch', () => {
    const baseFunc = (params?: RegexUtilParams) => buildRegex(type, params);
    const Helper = new RegexTestUtil('Ability', examples['Ability'], baseFunc, true);
    Helper.run();
    const testLine =
      '[20:29:39.392] ActionEffect 15:107B9AC8:Tako Yaki:07:Attack:40017D58:Daxio:710003:DC0000:1E:50000:1C:1B60000:550003:2CA000:0:0:0:0:0:0:0:0:8207:24837:7230:7230:0:1000:527.5806:-362.7833:-19.61513:2.898741:8998:8998:10000:10000:0:1000:528.6487:-365.8656:-22.08109:-0.3377206:000AD7BF:0:1';
    Helper.captureTest((params?: RegexUtilParams) => Regexes.ability(params), [testLine]);
    const abilityHallowed = Regexes.ability({ id: '1E' });
    assert.isNull(testLine.match(abilityHallowed));
  });

[valarnin]

A regex search of the codebase using ^(\s*)it\(.*?\n(?:\1 [^\n]*?\n|\n)*(?:\1 \s*it) didn't turn up any other cases, but this obviously isn't exhaustive.

edit:

There's an eslint plugin and rule to detect this, https://www.npmjs.com/package/eslint-plugin-mocha and mocha/no-nested-tests, but that requires eslint 9 which is newer than main.

We could maybe look at adding this to the 8.0-breaking-changes codebase after #1099 lands, and if so, maybe we fix this test on that branch/codebase since the test passes if it's moved up currently?

[Bing-su]

I was experimenting with migrating the test framework from Mocha to Vitest. I discovered this issue when Vitest rejected nested tests during the collection phase.

Please feel free to correct it as you see fit.

[valarnin]

I'm working on migrating webpack to vite as part of #907, it probably makes sense to migrate mocha to vitest afterwards if we do so?

[Bing-su]

The test time, which was over a minute before, was reduced to 15 seconds. It was a pretty good experience.

[Bing-su]

https://github.com/Bing-su/cactbot/tree/feat/vitest
My vitest experiment is here, so feel free to use it.