-
Notifications
You must be signed in to change notification settings - Fork 14
Use describe names for the suite name #198
base: main
Are you sure you want to change the base?
Conversation
@ryparker can you review PR |
Thanks for the PR. I'll make some time this weekend to follow up on this PR and other issues. I'm planning a move to Seattle so time has been limited. If anyone is interested in being added as a maintainer for this project LMK. Thanks again @alexneo2003 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Support for describe
blocks is definitely going to improve adoptability of this project however we don't want to break existing support for tests that do not use describe
.
Going to deny this implementation since it breaks functionality but I left comments on ideas of how we should implement this.
I appreciate your willingness to contribute to the project. Look forward to hearing your thoughts.
allure.attachment('HTML attachment', '<div><p>This is an HTML doc</p></div', ContentType.HTML); | ||
|
||
expect(1 + 2).toBe(3); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are still valuable for use cases where users have tests that do not use describe
to wrap their tests. We should keep these as is and instead add new tests that wrap them with describe
src/allure-reporter.ts
Outdated
|
||
if (suites.length > 0) { | ||
currentTest.addLabel(LabelName.SUITE, suites.join(' > ')); | ||
private addSuiteLabelsToTestCase(currentTest: AllureTest, parent: jest.Circus.DescribeBlock): AllureTest { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After testing this with the original tests by running yarn test
and then allure serve
I noticed that this new implementation does not support tests that are not wrapped in describe
which would be breaking current functionality. We should be able to support tests that are wrapped either with or without the describe
blocks.
I also noticed that the test file name is not the highest layer of the tests report. To match other Allure library implementations this would be something we should support. Especially for cases where there are multiple describe blocks with the same name.
One interesting way i've seen this implemented is in the Allure Jasmine library. They solve this by collecting the describe
layers via Jasmine's suiteStarted
and suiteDone
hooks. The equivalent of this hook would be the start_describe_definition
and finish_describe_definition
jest environment events.
Something like this could allow us to detect wether the test is in fact wrapped in a describe
block. Then we can collect the layers of describe block. Then once the finish_describe_definition
is called for that block we would remove it from the collection. All while supporting nested describe
blocks, and not effecting current implementation that supports tests without describe
blocks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file name is not the highest layer of the tests report
for example, I have very long spec files and for convenience I divide them into several files, but the tests in these files have a common describe and in the report I would like to see all these tests in one describe group
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, but my knowledge is probably not enough to implement this in the way you suggest using the start_describe_definition and finish_describe_definition events))
i'm trying to combine mine implementation with youth
i'm added extra check for tests without describe and if it exists - use old logic with file path
private addSuiteLabelsToTestCase(currentTest: AllureTest, parent: jest.Circus.DescribeBlock, testPath: string): AllureTest {
const isWindows = os.type() === 'Windows_NT';
const pathDelimiter = isWindows ? '\\' : '/';
const pathsArray = testPath.split(pathDelimiter);
const [parentSuite, ...suites] = pathsArray;
const subSuite = suites.pop();
if ((parent?.parent as any)?.parent?.parent && (parent?.parent as any)?.parent?.parent.name === 'ROOT_DESCRIBE_BLOCK') {
currentTest.addLabel(LabelName.PARENT_SUITE, (parent.parent as any).parent.name);
currentTest.addLabel(LabelName.SUITE, (parent.parent as any).name);
currentTest.addLabel(LabelName.SUB_SUITE, parent.name);
} else if ((parent?.parent as any)?.parent && (parent?.parent as any)?.parent.name === 'ROOT_DESCRIBE_BLOCK') {
currentTest.addLabel(LabelName.PARENT_SUITE, (parent.parent as any).name);
currentTest.addLabel(LabelName.SUITE, (parent as any).name);
} else if ((parent as any)?.parent && (parent as any)?.parent.name === 'ROOT_DESCRIBE_BLOCK') {
currentTest.addLabel(LabelName.PARENT_SUITE, (parent as any).name);
} else {
if (parentSuite) {
currentTest.addLabel(LabelName.PARENT_SUITE, parentSuite);
currentTest.addLabel(LabelName.PACKAGE, parentSuite);
}
if (suites.length > 0) {
currentTest.addLabel(LabelName.SUITE, suites.join(' > '));
}
if (subSuite) {
currentTest.addLabel(LabelName.SUB_SUITE, subSuite);
}
}
return currentTest;
}
it will look like
can you check it and if this option is suitable I will make a commit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ryparker can you take me any feedback
HI @ryparker /@alexneo2003 any update about this fix? By when this feature will be available? |
@ryparker answer me please |
Hi all yarn add @alex_neo/jest-circus-allure-environment jest.config.js testEnvironment: '@alex_neo/jest-circus-allure-environment', |
added features requested at issue #179