feat(reporter): add support for function type to classname option in the junit reporter #6839

jpleclerc · 2024-11-01T22:46:36Z

Description

This PR adds support for a function for the classname option for the junit reporter.
It is currently possible to specify a static string as the classname of the junit reporter but it is not flexible enough in some case.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. If the feature is substantial or introduces breaking changes without a discussion, PR might be closed.
Ideally, include a test that fails without this PR but passes with it.
Please, don't make changes to pnpm-lock.yaml unless you introduce a new test example.

Tests

Run the tests with pnpm test:ci.

Documentation

If you introduce new functionality, document it. You can run documentation with pnpm run docs command.

Changesets

Changes in changelog are generated from PR name. Please, make sure that it explains your changes in an understandable manner. Please, prefix changeset messages with feat:, fix:, perf:, docs:, or chore:.

…on in the junit reporter

hi-ogawa · 2024-11-02T01:26:00Z

Hey, thanks for the PR. Can you explain your specific use case? While exposing Task gives flexibility, that comes with increased API surface and potential breaking change in the future, so we might need to discuss a bit.

As an alternative, would it work for you if we support classNameTemplate like jest's reporter? https://github.com/jest-community/jest-junit?tab=readme-ov-file#configuration They have functions too https://github.com/jest-community/jest-junit?tab=readme-ov-file#example-5, but it looks like the argument is normalized first, so internal is not exposed.

Btw, what we support right now for options.classname is probably weird/useless #3808 as it looks like OP introduced to just hide classname #3691

jpleclerc · 2024-11-02T07:01:46Z

We ended up having test cases with duplicated class names in our project and our CI/CD reporting UI couldn't handle duplicates. For that reason we need a more flexible way to generate classnames in order to make them more unique.

Exposing Task definitly felt a bit wrong. I just looked at jest's classNameTemplate and definitly feels cleaner. I'll make the change to mimic that behavior.

Current classname option feels useless indeed, can't see a use for that.

AriPerkkio · 2024-11-02T08:18:24Z

@jpleclerc what properties of Task would you like to use here to create those unique names? Instead of exposing whole Task, we could expose some specific parts that are easier to maintain.

jpleclerc · 2024-11-04T15:40:30Z

@AriPerkkio In our case the file name and path is enough. I think the jest-junit project got it right. I'm thinking of supporting at least the following:

suitename
filepath
filename

- classnameTemplate can take a function or a string - the string can be parameterized Change-Id: I6a79ed8f89e23e518c8d18f6fe49a210edaa247a

netlify · 2024-11-04T16:45:52Z

✅ Deploy Preview for vitest-dev ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`2e3dc97`
🔍 Latest deploy log	https://app.netlify.com/sites/vitest-dev/deploys/67327ed7ca8e7f0008298295
😎 Deploy Preview	https://deploy-preview-6839--vitest-dev.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

… classnameTemplate Change-Id: I03a3d47d26aa36ca5c14cf790a19629d151b7c61

hi-ogawa

Looks good!
I think we can deprecate (and remove later) classname as it's useless and easy to move to classnameTemplate as a constant.

packages/vitest/src/node/reporters/junit.ts

AriPerkkio

Looks good! We should also update documentation. Maybe something about classnameTemplate after this section:

vitest/docs/guide/reporters.md

Lines 252 to 262 in 5d4b382

    
           The outputted XML contains nested `testsuites` and `testcase` tags. You can use the reporter options to configure these attributes: 
        
           ```ts 
        
           export default defineConfig({ 
        
             test: { 
        
               reporters: [ 
        
                 ['junit', { suiteName: 'custom suite name', classname: 'custom-classname' }] 
        
               ] 
        
             }, 
        
           }) 
        
           ```

Change-Id: I1caa6e359998071ced9839ad27d121d25e8b8d99

docs/guide/reporters.md

packages/vitest/src/node/reporters/junit.ts

sheremet-va · 2024-11-08T09:11:52Z

packages/vitest/src/node/reporters/junit.ts

+      const templateVars: ClassnameTemplateVariables = {
+        filename: task.file.name,
+        filepath: task.file.filepath,
+        suitename: this.options.suiteName ?? task.suite?.name ?? '',


what should be a suitename if the test is declared at the top level? Currently, suite there is undefined. Is '' (empty string) intended here?

I'm not sure if empty string is the way to go here. I'm open to suggestions.

options.suiteName is about <testsuites name="vitest tests">, so that shouldn't come first.

Also Jest's "suite name" concept is a bit different while Vitest simply uses <testsuite name=(filepath)>, so we might not even include suitename here in ClassnameTemplateVariables. I vote for removing it if it filename/filepath is enough for OP's use case.

… fixed typo. Change-Id: Ie77e0955011fa42d2a8ef42c074327589d780739

feat(reporter): added support function type support to classname opti…

e774ead

…on in the junit reporter

jpleclerc changed the title ~~feat(reporter): added support function type support to classname option in the junit reporter~~ feat(reporter): add support function type support to classname option in the junit reporter Nov 1, 2024

jpleclerc changed the title ~~feat(reporter): add support function type support to classname option in the junit reporter~~ feat(reporter): add support for function type to classname option in the junit reporter Nov 1, 2024

feat(reporter): created a new option for the junit reporter.

b061706

- classnameTemplate can take a function or a string - the string can be parameterized Change-Id: I6a79ed8f89e23e518c8d18f6fe49a210edaa247a

feat(reporter): default to options.suiteName for the suitename in the…

9bad3c8

… classnameTemplate Change-Id: I03a3d47d26aa36ca5c14cf790a19629d151b7c61

hi-ogawa previously approved these changes Nov 4, 2024

View reviewed changes

packages/vitest/src/node/reporters/junit.ts Show resolved Hide resolved

chore: deprecate classname

23448c0

hi-ogawa dismissed their stale review via 23448c0 November 4, 2024 23:32

hi-ogawa previously approved these changes Nov 4, 2024

View reviewed changes

AriPerkkio requested changes Nov 5, 2024

View reviewed changes

feat(reporter): Mention classnameTemplate in the doc

04add0c

Change-Id: I1caa6e359998071ced9839ad27d121d25e8b8d99

jpleclerc dismissed hi-ogawa’s stale review via 04add0c November 7, 2024 17:44

AriPerkkio previously approved these changes Nov 7, 2024

View reviewed changes

sheremet-va added this to the 2.2.0 milestone Nov 8, 2024

sheremet-va reviewed Nov 8, 2024

View reviewed changes

feat(reporter): Sightly improved documentation for junit reporter and…

2e3dc97

… fixed typo. Change-Id: Ie77e0955011fa42d2a8ef42c074327589d780739

jpleclerc dismissed AriPerkkio’s stale review via 2e3dc97 November 11, 2024 22:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(reporter): add support for function type to classname option in the junit reporter #6839

feat(reporter): add support for function type to classname option in the junit reporter #6839

jpleclerc commented Nov 1, 2024 •

edited

Loading

hi-ogawa commented Nov 2, 2024 •

edited

Loading

jpleclerc commented Nov 2, 2024 •

edited

Loading

AriPerkkio commented Nov 2, 2024

jpleclerc commented Nov 4, 2024

netlify bot commented Nov 4, 2024 •

edited

Loading

hi-ogawa left a comment

AriPerkkio left a comment

sheremet-va Nov 8, 2024

jpleclerc Nov 11, 2024

hi-ogawa Nov 12, 2024 •

edited

Loading

	The outputted XML contains nested `testsuites` and `testcase` tags. You can use the reporter options to configure these attributes:

	```ts
	export default defineConfig({
	test: {
	reporters: [
	['junit', { suiteName: 'custom suite name', classname: 'custom-classname' }]
	]
	},
	})
	```

feat(reporter): add support for function type to classname option in the junit reporter #6839

Are you sure you want to change the base?

feat(reporter): add support for function type to classname option in the junit reporter #6839

Conversation

jpleclerc commented Nov 1, 2024 • edited Loading

Description

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Documentation

Changesets

hi-ogawa commented Nov 2, 2024 • edited Loading

jpleclerc commented Nov 2, 2024 • edited Loading

AriPerkkio commented Nov 2, 2024

jpleclerc commented Nov 4, 2024

netlify bot commented Nov 4, 2024 • edited Loading

✅ Deploy Preview for vitest-dev ready!

hi-ogawa left a comment

Choose a reason for hiding this comment

AriPerkkio left a comment

Choose a reason for hiding this comment

sheremet-va Nov 8, 2024

Choose a reason for hiding this comment

jpleclerc Nov 11, 2024

Choose a reason for hiding this comment

hi-ogawa Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

jpleclerc commented Nov 1, 2024 •

edited

Loading

hi-ogawa commented Nov 2, 2024 •

edited

Loading

jpleclerc commented Nov 2, 2024 •

edited

Loading

netlify bot commented Nov 4, 2024 •

edited

Loading

hi-ogawa Nov 12, 2024 •

edited

Loading