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

feat: introduce __mocha_worker_id__ property #4922

Closed
wants to merge 4 commits into from

Conversation

epszaw
Copy link

@epszaw epszaw commented Sep 14, 2022

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions.

Description of the Change

The PR brings __mocha_worker_id__ property to tests, suites and hooks, just to understand on reporter level, which worker has been used and categorize multiple parallel tests on reporter level.

At this moment, many solutions are compatible only with the serial mode and when we try to run parallel one current test/suite/hook will be overwritten.

This solution doesn't bring something, which can break the runner or any solution based on it because it adds __mocha_worker_id__ field what has MOCHA_WORKER_ID value (or it equals to undefined, e.g. in serial mode).

Alternate Designs

I've tried to get MOCHA_WORKER_ID during allure-mocha reporter development, but it's not possible to get it due the reporter receives only IPC message.

Why should this be in core?

Many people run mocha in parallel mode and they want to use their own or third-party reporters which are compatible only with serial mode, so we can't store any concurrent data inside. If we have worker ID, we would store running tests by it without any collision.

Benefits

We can adapt any reporter to work in parallel mode. The main motivation – make allure-mocha work.

Possible Drawbacks

__mocha_worker_id__ can be undefined in the serial mode. Maybe it worth to omit the field, but from my vision, better to keep it all the time.

Applicable issues

#4768

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 14, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@epszaw
Copy link
Author

epszaw commented Sep 27, 2022

@juergba could someone check this? How it could be merged?

@vovsemenv
Copy link

@juergba we really need this 👻

@epszaw
Copy link
Author

epszaw commented Oct 21, 2022

@outsideris could you please leave a review here? :)

@juergba
Copy link
Contributor

juergba commented Oct 23, 2022

@epszaw I want to understand this issue before taking any decision. Adding some properties by reasoning it wouldn't break anything, doesn't convince at the moment. There are also a few custom reporters working in parallel mode. Some additional information by your side could help.

  • " [...] test/suite/hook will be overwritten": don't we have an unique ID per suite/test/hook?
  • the workers could be re-used (pool), with the same MOCHA_WORKER_ID: implications for your proposal?
  • "__mocha_worker_id__ can be undefined in the serial mode": you set this property only when serializing, right?

Could you read this issue #4403, please? Is it the same kind of issue?

@baev
Copy link

baev commented Oct 23, 2022

the main idea is that Allure reporter inits state and then users may modify the state within a test:

it('simple test', function () {
    allure.step('check A',  function () {
        //...
    })
    allure.step('check B',  function () {
        //...
    })
});

Allure provides multiple features that allows users to enrich their tests reports with additional metadata such as steps, attachments, tags, labels and so on. All that is usually implemented by custom methods that interact with reporter's state.
When tests run in parallel mode, we can create a message bus to interact from worker to the parent process (by using files, sockets, events or some other way). But we need somehow to separate events that come from different workers.

Besides that Allure offer Timeline feature -- that shows the distribution of the tests between hosts and workers:

image

@epszaw
Copy link
Author

epszaw commented Nov 17, 2022

@juergba any news here?

@epszaw
Copy link
Author

epszaw commented Dec 12, 2022

@juergba could you please provide any news here?

@epszaw epszaw closed this Feb 17, 2023
@Rogatto
Copy link

Rogatto commented Jun 30, 2023

Any news about this issue?

Using allure methods inside a global fixtures there is no error:


export const mochaGlobalTeardown = async () => {
  type Environments = Record<string, string>

  const environments: Environments = {
      environment
  }

  allure.writeEnvironmentInfo(environments)
  console.log('Finishing execution!');
}

The problem occurs here at the test spec:

import { allure } from 'allure-mocha/runtime'

beforeEach(function () {
    allure.tms(`[${scenarioId}]`, routeTestManagement + scenarioId)
    allure.severity(Severity.CRITICAL)
    allure.feature(feature)
  })
 "before each" hook for "it must create a new product":
  TypeError: runtime_1.allure.tms is not a function

@epszaw
Copy link
Author

epszaw commented Jun 30, 2023

Any news about this issue?

Using allure methods inside a global fixtures there is no error:


export const mochaGlobalTeardown = async () => {
  type Environments = Record<string, string>

  const environments: Environments = {
      environment
  }

  allure.writeEnvironmentInfo(environments)
  console.log('Finishing execution!');
}

The problem occurs here at the test spec:

import { allure } from 'allure-mocha/runtime'

beforeEach(function () {
    allure.tms(`[${scenarioId}]`, routeTestManagement + scenarioId)
    allure.severity(Severity.CRITICAL)
    allure.feature(feature)
  })
 "before each" hook for "it must create a new product":
  TypeError: runtime_1.allure.tms is not a function

If you have a problem with allure reporter, please, open an issue in the repo

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

Successfully merging this pull request may close these issues.

5 participants