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_runner: improve describe.only behavior #52296

Merged
merged 3 commits into from
Apr 2, 2024
Merged

test_runner: improve describe.only behavior #52296

merged 3 commits into from
Apr 2, 2024

Conversation

MoLow
Copy link
Member

@MoLow MoLow commented Mar 31, 2024 •
edited
Loading

Supersedes #48932

this fixes two major issues with describe that will now run regardless of it not being marked with only:

  1. if a test is marked with only, it is not needed to mark all of its ancestors with only
  2. if a suite is marked as only, and no nested test is marked with only, all its decendents will run

this aligns the behavior with other test runners I have compared with

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem. labels Mar 31, 2024
@MoLow
Copy link
Member Author

MoLow commented Mar 31, 2024

some examples:

describe('1', () => {
  it('2');
  describe('3', () => {
    it('4');
    describe('5', () => {
      it('6');
      it.only('7');
    });
  });
});

===>

▶ 1
  ▶ 3
    ▶ 5
      ✔ 7 (0.30825ms)
    ▶ 5 (0.813458ms)
  ▶ 3 (1.103583ms)
▶ 1 (1.578209ms)

describe('1', () => {
  it('2');
  describe.only('3', () => {
    it('4');
    describe('5', () => {
      it('6');
      it('7');
    });
  });
});

===>

▶ 1
  ▶ 3
    ✔ 4 (0.414666ms)
    ▶ 5
      ✔ 6 (0.070375ms)
      ✔ 7 (0.112041ms)
    ▶ 5 (0.339458ms)
  ▶ 3 (1.070458ms)
▶ 1 (1.617208ms)

etc

cjihrig
cjihrig approved these changes Mar 31, 2024
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM, but it's probably worth updating some docs for this.

test/fixtures/test-runner/output/only_tests.js Outdated Show resolved Hide resolved
cjihrig
cjihrig approved these changes Apr 1, 2024
View reviewed changes
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Outdated Show resolved Hide resolved
doc/api/test.md Show resolved Hide resolved
@MoLow MoLow requested a review from atlowChemi April 1, 2024 18:47
@MoLow MoLow added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 1, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58037/

@cjihrig cjihrig mentioned this pull request Apr 1, 2024
Closed
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58039/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/58040/

@MoLow MoLow added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Apr 1, 2024
rluvaton
rluvaton reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@rluvaton rluvaton left a comment

Choose a reason for hiding this comment

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

Can you add a test that before and after hooks are not running for unfocused suites? IIRC they do run when there are no tests in describe while I think they shouldn't in this case

@MoLow
Copy link
Member Author

MoLow commented Apr 2, 2024 •
edited
Loading

@rluvaton can you add these tests in a follow-up PR?. I am not sure what an unfocused suite is

@rluvaton
Copy link
Member

rluvaton commented Apr 2, 2024

sure (what I meant with unfocused are suites that or not in the only scope)

@MoLow
Copy link
Member Author

MoLow commented Apr 2, 2024

FWIW this code

describe('describe 1', () => {
  before(() => console.log('before describe 1'))
  beforeEach(() => console.log('beforeEach describe 1'))
  it.only(async () => {
    console.log(1);
  })
  it('no', () => {});
  afterEach(() => console.log('afterEach describe 1'))
  after(() => console.log('after describe 1'))
})


describe('describe 2', () => {
  before(() => console.log('before describe 2'))
  beforeEach(() => console.log('beforeEach describe 2'))
  it('no', () => {});
  afterEach(() => console.log('afterEach describe 2'))
  after(() => console.log('after describe 2'))
})

outputs

before describe 1
beforeEach describe 1
1
afterEach describe 1
after describe 1
▶ describe 1
  ✔ <anonymous> (0.769792ms)
▶ describe 1 (5.330458ms)

which seems ok to me
CI is locked down so I prefer not to perform additional changes in this branch

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 2, 2024
@nodejs-github-bot nodejs-github-bot merged commit ac9e5e7 into nodejs:main Apr 2, 2024
62 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in ac9e5e7

@MoLow MoLow deleted the test-runner-only-improve branch April 3, 2024 06:36
@RafaelGSS RafaelGSS mentioned this pull request Apr 15, 2024
Merged
@marco-ippolito
Copy link
Member

Can you please create a manual backport to v20x?

@marco-ippolito marco-ippolito added the backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. label May 21, 2024
@MoLow MoLow mentioned this pull request May 24, 2024
Closed
@MoLow MoLow added backport-open-v20.x Indicate that the PR has an open backport and removed backport-requested-v20.x PRs awaiting manual backport to the v20.x-staging branch. labels May 24, 2024
@MoLow MoLow mentioned this pull request Jun 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rluvaton rluvaton rluvaton left review comments

@atlowChemi atlowChemi atlowChemi approved these changes

@trivikr trivikr trivikr approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v20.x Indicate that the PR has an open backport commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. test_runner Issues and PRs related to the test runner subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@MoLow @nodejs-github-bot @rluvaton @marco-ippolito @benjamingr @cjihrig @trivikr @atlowChemi