Skip to content

fix cucumber junit report#11131

Merged
christian-bromann merged 3 commits intowebdriverio:mainfrom
tamil777selvan:cucumber_fix_junit
Sep 12, 2023
Merged

fix cucumber junit report#11131
christian-bromann merged 3 commits intowebdriverio:mainfrom
tamil777selvan:cucumber_fix_junit

Conversation

@tamil777selvan
Copy link
Member

@tamil777selvan tamil777selvan commented Sep 11, 2023

Proposed changes

Fix - #11125

It looks at the URI that comes from the cucumber event broadcaster to see if it's an absolute path. If it's absolute, it just uses that URI. If not then it figures out the full path and uses it.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Checklist

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have added proper type definitions for new commands (if appropriate)

Further comments

Reviewers: @webdriverio/project-committers

this.onTestRunStarted()
} else if (envelope.pickle) {
this.onPickleAccepted(envelope.pickle)
this.onPickleAccepted({ ...envelope.pickle, uri: this.normalizeURI(envelope.pickle.uri)!! })
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.onPickleAccepted({ ...envelope.pickle, uri: this.normalizeURI(envelope.pickle.uri)!! })
this.onPickleAccepted({ ...envelope.pickle, uri: this.normalizeURI(envelope.pickle.uri) })

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason I added !! is that normalizeURI should not be left as undefined for envelope.pickle.uri. Without this, it would cause issues with data types.

Copy link
Member

Choose a reason for hiding this comment

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

This seems to me we just suppress the case where normalizeURI would return undefined. How about we change its interface to this:

    normalizeURI(uri: string) {
        return path.isAbsolute(uri) ? uri : path.resolve(uri)
    }

given that envelope.pickle.uri is always defined as string.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with your suggestion to modify the normalizeURI function as proposed above. I'll go ahead and implement those changes now.

Copy link
Member Author

@tamil777selvan tamil777selvan Sep 11, 2023

Choose a reason for hiding this comment

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

My apologies, GherkinDocument has a type uri?: string, and pickle has type as uri: string. This modification cause type conflicts with GherkinDocument. Please share your opinion on this.

Copy link
Member

Choose a reason for hiding this comment

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

for the onGherkinDocument call we could do:

                this.onGherkinDocument({
                    ...envelope.gherkinDocument,
                    ...(envelope.gherkinDocument.uri
                        ? { uri: this.normalizeURI(envelope.gherkinDocument.uri) }
                        : {}
                    )
                })

Copy link
Member

@christian-bromann christian-bromann left a comment

Choose a reason for hiding this comment

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

Awesome 👍

Really great work over the last 2 weeks or so. I am already talking with the other contributor to have you added as a member once you make 2 more PRs and reach 10 impactful contributions 🎉

@christian-bromann christian-bromann added the PR: Bug Fix 🐛 PRs that contain bug fixes label Sep 12, 2023
@christian-bromann christian-bromann merged commit a77215f into webdriverio:main Sep 12, 2023
@tamil777selvan
Copy link
Member Author

Awesome 👍

Really great work over the last 2 weeks or so. I am already talking with the other contributor to have you added as a member once you make 2 more PRs and reach 10 impactful contributions 🎉

Thanks! I'm glad you liked my work in the past couple of weeks. I'll aim to do two more PRs and reach 10 impactful contributions as you've suggested. Exciting times ahead! 🚀🎉

@tamil777selvan tamil777selvan deleted the cucumber_fix_junit branch September 12, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Bug Fix 🐛 PRs that contain bug fixes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants