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.

Sign up for GitHub

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

Jump to bottom

Option to disable screenshot data in logs #262

Closed
wants to merge 1 commit into from
Closed

Option to disable screenshot data in logs #262

wants to merge 1 commit into from

Conversation

avaly
Copy link
Contributor

@avaly avaly commented Sep 10, 2014

Setting the option log_screenshot_data in one of the test_sestings environment
causes the screenshot base64 data to be outputted or hidden from the logs.

Fixed #256

@avaly
Copy link
Contributor Author

avaly commented Sep 10, 2014

The test assertions could be written a bit cleaner with Sinon.js, but I didn't want to add such a dependency only for two new tests.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) when pulling 41f9bef on avaly:feature/disable-screenshot-data-in-logs into f231048 on beatfactor:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling fb98c54 on avaly:feature/disable-screenshot-data-in-logs into f231048 on beatfactor:master.

@beatfactor
Copy link
Member

instead of nock do you mean? I thought sinon is for the browser. Maybe log_screenshot_data would be a more suggestive property name?

@avaly
Copy link
Contributor Author

avaly commented Sep 10, 2014

OK, I'll update the setting name to log_screenshot_data tomorrow.

No, using sinon for asserting that Logger.info was called corrrectly. So, instead of line 118-122 on the request test file, you could write that as:

sinon.stub(Logger, 'info');
// do stuff
test.deepEqual(Logger.info.lastCall.args[1], {...});

@avaly
Copy link
Contributor Author

avaly commented Sep 11, 2014

Updated setting name.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) when pulling 20db959 on avaly:feature/disable-screenshot-data-in-logs into f231048 on beatfactor:master.

beatfactor
beatfactor reviewed Sep 11, 2014
View reviewed changes
lib/http/request.js
@@ -90,8 +90,7 @@ module.exports = (function() {
});

response.on('end', function () {
var screenshotContent;
var result, errorMessage = '';
var screenshotContent, result, resultLog = {}, errorMessage = '';
Copy link
Member

Choose a reason for hiding this comment

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

I'm not really in favour of the one-line var statement style as the standard throughout the project is one variable per line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You should use npm module jscs to enforce the preferred style guide for this project and husky to enforce running such a code style check on pre-commit or pre-push.

@avaly
Copy link
Contributor Author

avaly commented Sep 12, 2014

Updated code based on code style requests. Also changed the implementation to overwrite the value attribute of the result object instead of just removing them.

@avaly
Option to disable screenshot data in logs
fdc5079 
Setting the option `screenshot_data` in one of the `test_sestings` environment
causes the screenshot base64 data to be outputted or hidden from the logs.

Fixes #256
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) when pulling fdc5079 on avaly:feature/disable-screenshot-data-in-logs into 4c756db on beatfactor:master.

@avaly
Copy link
Contributor Author

avaly commented Sep 15, 2014

There seems to be some funkyness with Travis builds. But this branch passes all tests.

@avaly
Copy link
Contributor Author

avaly commented Sep 16, 2014

Any chance on getting this merged soon?

beatfactor added a commit that referenced this pull request Sep 16, 2014
@beatfactor
Added option to disable screenshot data in logs - based on #262 and a…
8902bbb 
…dded context for global before and after hooks - based on #264
@beatfactor
Copy link
Member

I changed the implementation slightly but it does the same thing. Hope that's ok.

@avaly
Copy link
Contributor Author

avaly commented Sep 17, 2014

Sure, no problem. Thanks!

@avaly avaly closed this Sep 17, 2014
@avaly
Copy link
Contributor Author

avaly commented Sep 17, 2014

Would be good to have this setting documented as well.

@beatfactor
Copy link
Member

Sure, it will be added soon.

On Wed, Sep 17, 2014 at 12:31 PM, Valentin Agachi notifications@github.com
wrote:

Would be good to have this setting documented as well.

—
Reply to this email directly or view it on GitHub
https://github.com/beatfactor/nightwatch/pull/262#issuecomment-55875919.

beatfactor added a commit that referenced this pull request Sep 21, 2014
@beatfactor
Merge branch 'master' of github.com:beatfactor/nightwatch
36ee8ed 
* 'master' of github.com:beatfactor/nightwatch:
  0.5.26
  Fixed #274 - screenshot data still displayed in logs
  0.5.25
  added option to automatically send the test module name to the selenium server in the desiredCapabilities - based on #254
  Added option to disable screenshot data in logs - based on #262 and added context for global before and after hooks - based on #264
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Disable screenshot data logging
4 participants
@avaly @coveralls @beatfactor @davidlinse