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

Added ability to add console log to allure and spec reporters (fix for #7001) #7134

Merged
merged 12 commits into from
Jul 15, 2021

Conversation

praveendvd
Copy link
Contributor

@praveendvd praveendvd commented Jul 13, 2021

Proposed changes

This commit allows to add cosnole logs to spec and allure reporters , all webdriverio logs containing mwebdriver will be ignored to avoid overloading the console log being logged (fix for #7001)

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

@christian-bromann

@praveendvd
Copy link
Contributor Author

praveendvd commented Jul 13, 2021

@christian-bromann i am not able to understand how to write unit test for spec reporter , could you have a look at : https://github.com/webdriverio/webdriverio/pull/7134/files#diff-cfe87f6281a8a4048d929804c6a8caf9eae3031fe3af66296e2f58d0a46cbde5

Its not live report , it seems like the result are being mocked so i am not sure how to mock presence of console.log

Copy link
Contributor Author

@praveendvd praveendvd left a comment

Choose a reason for hiding this comment

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

tmpReporter.onSuiteStart(suiteEnd()) was corrected to tmpReporter.onSuiteEnd()

packages/wdio-allure-reporter/src/index.ts Outdated Show resolved Hide resolved
packages/wdio-allure-reporter/src/types.ts Outdated Show resolved Hide resolved
packages/wdio-allure-reporter/src/utils.ts Outdated Show resolved Hide resolved
packages/wdio-spec-reporter/src/index.ts Outdated Show resolved Hide resolved
@@ -45,6 +50,11 @@ export default class SpecReporter extends WDIOReporter {
this._sauceLabsSharableLinks = 'sauceLabsSharableLinks' in options
? options.sauceLabsSharableLinks as boolean
: this._sauceLabsSharableLinks
let processObj: any = process
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
let processObj: any = process
let processObj = process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if i remove any , i get
image

packages/wdio-spec-reporter/src/index.ts Outdated Show resolved Hide resolved
@praveendvd praveendvd closed this Jul 14, 2021
@praveendvd praveendvd reopened this Jul 14, 2021
@praveendvd praveendvd closed this Jul 14, 2021
@christian-bromann
Copy link
Member

Why closing the PR?

@praveendvd
Copy link
Contributor Author

@christian-bromann making changes didn't wanted each push to trigger the work flow and spam your inbox , thanks for the detailed review

@christian-bromann
Copy link
Member

Don't worry about that 😉

@praveendvd praveendvd reopened this Jul 14, 2021
@praveendvd
Copy link
Contributor Author

@christian-bromann , i done all i could i am not able to increase statement coverage Jest: "global" coverage threshold for statements (98%) not met: 97.98% 😢

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.

Couple more comments, feel free to drop the coverage by 1% 😉

@@ -45,6 +50,16 @@ export default class SpecReporter extends WDIOReporter {
this._sauceLabsSharableLinks = 'sauceLabsSharableLinks' in options
? options.sauceLabsSharableLinks as boolean
: this._sauceLabsSharableLinks
let processObj:any = process
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
let processObj:any = process
let processObj = process

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if i remove any , i get
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christian-bromann could you have a look at this , what type should i give

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@christian-bromann If no changes needs to be done for this comment then could you retrigger the workflow it looks like an intermittent failure. sleep(100) was 99 something, Thank you so much for your time and detailed reviews learned a lot

packages/wdio-spec-reporter/src/index.ts Outdated Show resolved Hide resolved
packages/wdio-spec-reporter/tests/index.test.ts Outdated Show resolved Hide resolved
@praveendvd
Copy link
Contributor Author

@christian-bromann i saw light at the end of the tunnel 😆 all green uff thank you Christian

@christian-bromann christian-bromann added the PR: Polish 💅 PRs that contain improvements on existing features label Jul 15, 2021
@christian-bromann christian-bromann merged commit 0a7887d into webdriverio:main Jul 15, 2021
@christian-bromann
Copy link
Member

Good job @praveendvd

@praveendvd praveendvd deleted the console-log branch July 15, 2021 10:36
@starbuck251
Copy link

@praveendvd we're using this in our runs, but the console logs are not added to the failing step. Our dev was able to fix it but unable to jump through the submission hooks. I wonder if you could look into fixing it? if you haven't already

@praveendvd
Copy link
Contributor Author

@praveendvd we're using this in our runs, but the console logs are not added to the failing step. Our dev was able to fix it but unable to jump through the submission hooks. I wonder if you could look into fixing it? if you haven't already

Could you add an reproducible example

@starbuck251
Copy link

Hi @praveendvd not without completely deconstructing our repo, I'd need to build one from scratch. I have asked my dev if he'd be able to comment on here as to how he fixed it - would that be of help? It will take me a couple of days before I will have time to build a small example, but will do so as the failing step is where we really need the logs :) - great work btw - its really useful!

@praveendvd
Copy link
Contributor Author

@starbuck251 Glad it's helpful , could you add one example code where Yoh explicitly add some console log and explicitly make the code fail but the log not getting added to the report

@starbuck251
Copy link

sure thing @praveendvd , will work on that over next couple of days and send a link to it :)

@praveendvd
Copy link
Contributor Author

@starbuck251 is the issue with cucumber ?

@starbuck251
Copy link

yes, we use the cucumber framework for our project

@praveendvd
Copy link
Contributor Author

@starbuck251

attachConsoleLogs(this._consoleOutput, this._allure)

moving this to top of onTestFail fixes the issue, was that the fix the dev team suggested ?

@starbuck251
Copy link

I believe it might be something like that, I'll slack him to confirm :)

@starbuck251
Copy link

image
Hi @praveendvd I got confirmation today - that your suggestion there is the same which will fix the issue of logs not attaching on failed steps :) which we tested here I believe
Will this be sufficient do you think or shall I work on an example repo also?

@praveendvd
Copy link
Contributor Author

image
Hi @praveendvd I got confirmation today - that your suggestion there is the same which will fix the issue of logs not attaching on failed steps :) which we tested here I believe
Will this be sufficient do you think or shall I work on an example repo also?

Just raise a issue will create a pull request today thank you

@starbuck251
Copy link

Thanks, that's amazingly helpful and kind of you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Polish 💅 PRs that contain improvements on existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants