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

The deno test --filter CLI option does not filter individual BDD tests #6063

Open
dandv opened this issue Sep 26, 2024 · 7 comments
Open

The deno test --filter CLI option does not filter individual BDD tests #6063

dandv opened this issue Sep 26, 2024 · 7 comments
Labels
bug Something isn't working testing

Comments

@dandv
Copy link
Contributor

dandv commented Sep 26, 2024

Describe the bug
During behavior-driven development it is useful to group tests into describes and also to be able to run tests individually. The test/t.step structure does not allow filtering for individual steps (and is not intended to). Therefore I resorted to the standard BDD library, but it turns our that filtering also only applies to the names of describe blocks, not test or it blocks.

Steps to Reproduce

  1. Create bdd.ts with the contents
import { describe, test, it } from 'jsr:@std/testing/bdd';
import { assertEquals } from '@std/assert';

describe('this is the describe', () => {
  test('test 1', () => {
    assertEquals(1, 1);
  });
  it('test 2', () => {
    assertEquals(2, 2);
  });
});
  1. Run deno test --filter /2/ bdd.ts
  2. Run deno test --filter "test 2" bdd.ts

Expected behavior

test 2 should execute.

Environment

  • OS: Fedora Linux 39
  • deno version: 1.46.3
  • std version: 1.0.3
@dandv dandv added bug Something isn't working needs triage labels Sep 26, 2024
@kt3k kt3k added testing upstream Changes upstream are required to solve these issues and removed needs triage labels Oct 9, 2024
@kt3k kt3k removed the upstream Changes upstream are required to solve these issues label Nov 23, 2024
@kt3k
Copy link
Member

kt3k commented Nov 23, 2024

This happens because we map each test/it case to test step (t.step) and test steps can't be filtered by deno test --filter option (This is by design in this way. See denoland/deno#26997 ).

This probably can be fixed by changing the mapping of it/test case to Deno.test instead of mapping to t.step.

@kt3k
Copy link
Member

kt3k commented Dec 3, 2024

@KyleJune I'm wondering if we can switch to map everything to Deno.test instead t.step because t.step doesn't (and probably won't) support the filtering and this issue can't be solved if we keep using t.step for nested test cases. Do you think it's possible/a good idea to do it?

@KyleJune
Copy link
Contributor

KyleJune commented Dec 4, 2024

@KyleJune I'm wondering if we can switch to map everything to Deno.test instead t.step because t.step doesn't (and probably won't) support the filtering and this issue can't be solved if we keep using t.step for nested test cases. Do you think it's possible/a good idea to do it?

I don't think that should be done. That would break a few things listed below:

  • Test output would no longer be grouped
  • Parent tests would finish before their children even start
  • Hooks would need to run after the parent test, before/after the child tests, preventing the parent test from catching leaking ops/resources from the hooks.
  • Parent test wouldn't catch leaking ops/resources not being cleaned up by hooks, making it harder to track down cascading test failures.

I have an idea for how we could support filtering by nested individual test cases or describe block names that would be similar to how we support only in nested test cases. However, it might require having a way for bdd to bypass the filter flag so that it can look at the flag and apply the filtering itself.

In TestSuiteInternal.run, it looks at 2 flags, suite.hasOnlyStep and suite.describe.only, which check if a child or the current test is focused using .only. We could do the same for filtering by having TestSuiteInternal override the only flag on the DescribeDefinition and ItDefinition for tests registered with describe and it when the --filter flag is present.

The change in bdd would be pretty straight forward since the existing code already checks if any steps have only set to true and will run the parent test if any of it's children have the only flag. All you would have to do is update describeDefinition and itDefinition in bdd.ts to check for a --filter flag. If the flag is present, check if test is not explicitly set to be ignored and that the name matches the filter, if it does match, set only to true. Then TestSuiteInternal would take care of the rest.

This idea depends on how filtering is implemented. If you use the filter flag, do tests registered with the only property set to true still run? If so, I think my proposed change would work, since the callback for describe is called immediately and bdd would handle adding only flag to any tests that need it. If filter applies first and tests focused using only are ignored, this would require a change so that bdd can handle how --filter applies to the test cases it registers.

I just tested this by creating the following test file and running deno test example.test.ts --filter "-1".

Deno.test({ name: "my-test", fn: () => {} });
Deno.test({ name: "test-1", fn: () => {} });
Deno.test({ name: "test-2", only: true, fn: () => {} });

It ended up only running "test-1" as shown in the following output.

$ deno test example.test.ts --filter "-1"
running 1 test from ./example.test.ts
test-1 ... ok (0ms)

ok | 1 passed | 0 failed | 2 filtered out (1ms)

So to actually get this to work, we are going to need a way for bdd to signal that --filter shouldn't apply to the tests it registers, that way it can apply the filtering itself via setting the only flag based on the filter flag.

One workaround that wouldn't require changing the test runner would be adding a separate flag for filtering bdd. For example, the change I suggested to bdd.ts could instead check for the flag --filter-bdd instead of --filter. The only downside to this approach is that it would be confusing having 2 separate filter flags depending on the style of test registration you are using. It would be preferable if bdd could just use the same --filter flag, it just needs to have a way to force tests to run despite not matching the filter.

@KyleJune
Copy link
Contributor

KyleJune commented Dec 4, 2024

With my proposal, I think the only difference between if a test is focused via the only property on the test definition vs the filter flag is that if you use the filter flag, is that it will count as a failure due to how tests are being focused. The screenshot shows the difference between test-2 being focused using the only property on the test definition vs test-1 being focused via a flag.

image

@kt3k
Copy link
Member

kt3k commented Dec 5, 2024

So to actually get this to work, we are going to need a way for bdd to signal that --filter shouldn't apply to the tests it registers, that way it can apply the filtering itself via setting the only flag based on the filter flag.

Does this mean we need to have Deno API for disabling native filtering behavior and also API for getting filtering string? I think that could be an option (At least we can start discussing that change in CLI repo), but I think that would take a bit of time..

One workaround that wouldn't require changing the test runner would be adding a separate flag for filtering bdd. For example, the change I suggested to bdd.ts could instead check for the flag --filter-bdd instead of --filter

This workaround also sounds fine to me.

@KyleJune
Copy link
Contributor

KyleJune commented Dec 5, 2024

So to actually get this to work, we are going to need a way for bdd to signal that --filter shouldn't apply to the tests it registers, that way it can apply the filtering itself via setting the only flag based on the filter flag.

Does this mean we need to have Deno API for disabling native filtering behavior and also API for getting filtering string? I think that could be an option (At least we can start discussing that change in CLI repo), but I think that would take a bit of time..

Yea, for Deno to hand over filtering control to std/bdd, we would need something like that. It wouldn't have to be completely disabling native filtering behavior, it could also be exposing a way to bypass the filter on an individual test bases. For example, if a new property was added to Deno.TestDefinition called ignoreFilterFlag, we would be able to update bdd to always set that to true on tests it registers so that it can control the filtering itself.

also API for getting filtering string?

I assumed accessing the flag is already possible via Deno.args. If the filter flag cannot be accessed, we would need a way to access it so that bdd can test each test name against the filter pattern to determine if only should be set to true.

If a code change is required to expose it, my recommendation would be to expose the filter pattern so that bdd can just use it directly instead of having to parse the text to determine if it is just a string or a regexp pattern (see https://docs.deno.com/runtime/fundamentals/testing/#filtering-by-string).

One workaround that wouldn't require changing the test runner would be adding a separate flag for filtering bdd. For example, the change I suggested to bdd.ts could instead check for the flag --filter-bdd instead of --filter

This workaround also sounds fine to me.

Yea, I thought it would be fine to do this workaround for now if someone wants to implement this feature, although it would be a bit hidden since the CLI wouldn't show that as a flag that could be used. However, I just did a quick test by logging Deno.args and it looks like we would actually need to do -- --filter-bdd "-1". I think this would likely cause confusion, so it's probably not worth implementing until we can access and use the --filter flag. Up to your team though on if the -- --filter-bdd "-1" workaround is acceptable.

image

@dandv
Copy link
Contributor Author

dandv commented Jan 9, 2025

Thanks everyone for taking a look at this. I've just noticed that only the outermost describe is filtered by. A nested describe test name won't be matched by the filter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

No branches or pull requests

3 participants