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

skip and only on the same level #1072

Closed
vonagam opened this issue Sep 9, 2023 · 5 comments
Closed

skip and only on the same level #1072

vonagam opened this issue Sep 9, 2023 · 5 comments

Comments

@vonagam
Copy link
Contributor

vonagam commented Sep 9, 2023

So let's say we have this:

const suite = create(() => {
  X

  test('field1', 'Field 1 is invalid', () => {throw 0});
  test('field2', 'Field 2 is invalid', () => {throw 0});
});

If X is skip('field1'); only('field1'); then there is no error - both skip and only are applied, no fields are run - but if the order is only('field1'); skip('field1'); then there is an error - only is applied and "field2" is skipped, but skip is not applied and "field1" runs.

Is this expected?

@ealush
Copy link
Owner

ealush commented Sep 9, 2023

This is interesting. which version are you using? The internal assumption in the code is that within a given scope, skip always takes precedence, and if a field is both skipped and only'd, we assume that the test should not run.

https://github.com/ealush/vest/blob/release/packages/vest/src/hooks/focused/useIsExcluded.ts#L33

@vonagam
Copy link
Contributor Author

vonagam commented Sep 9, 2023

I'm on 5.0.0-next-20230905-5c95. Here is a snippet to paste into vest runkit that shows errorCount: 1 on 5.0.2 version:

var vest = require("vest")

const suite = vest.create(() => {
  vest.only('field1'); 
  vest.skip('field1');

  vest.test('field1', 'Field 1 is invalid', () => {throw 0});
  vest.test('field2', 'Field 2 is invalid', () => {throw 0});
});

suite()

@ealush
Copy link
Owner

ealush commented Sep 9, 2023

OK, yeah. You are right. Re-read the whole part of the code that handles it and now it came back to me.

The way it currently works is as you described. When trying to run a test, vest traverses up from that test to the closest focus (either skip or only) declaration, and once found, it stops searching. Basically, if there are conflicting focus blocks, Vest picks the first one that's declared. The way the internal vest tree looks is somewhat like this:

{
  type: 'suite',
  optional: {},
  children: [
    {
      type: 'focus',
      mode: 'only',
    },
    {
      type: 'focus',
      mode: 'skip',
    },
    {
      type: 'test',
      testFn: () => { },
      fieldName: 'username',
      message: 'username is required',
    },
  ],
};

Now, this tree can be infinitely nested with group, each, skipWhen, and the other functions. It can also be infinitely dense.
When trying to understand if a given scope is focused, the search can get very expensive (here's the search I'm doing now: https://github.com/ealush/vest/blob/latest/packages/vest/src/hooks/focused/useIsExcluded.ts#L15, the result of this is handed over to the other function I shared previously), so the cheap solution is first-wins, and it should probably be documented.

I am wondering, what would be a use case in which you need both? We might either be able to achieve it in a different way, or find a way to support that within Vest.

@ealush
Copy link
Owner

ealush commented Sep 9, 2023

Oh, and by the way, Vest 5 is officially out since Thursday.

@vonagam
Copy link
Contributor Author

vonagam commented Sep 9, 2023

Vest picks the first one that's declared

Ok, so skip; only; is the correct order to make them work together. skip will be the first to apply to skipped fields and only will be the first applied to non-skipped fields.

Use case? This is about those disabled fields. When a field is disabled I can do "suite.remove", but when it comes back from being disabled and if I do not want the tests to run but still want it to affect validity I need to rerun the suite with "only" and "skip" for it.

@vonagam vonagam closed this as completed Sep 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants