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

Bug fix for occasional NPE's in SelenideReport #686

Merged
merged 2 commits into from Mar 13, 2018

Conversation

Projects
None yet
6 participants
@dkorobtsov
Contributor

dkorobtsov commented Jan 29, 2018

Proposed changes

Bug fix for occasional NPE's in SelenideReport when used with TestNg listeners.
On certain occasions - when SimpleReport.start() method was not called for any reason, SelenideReport.finish() was causing NPE.

Checklist

  • [+] Checkstyle and unit tests pass locally with my changes by running gradle check chrome htmlunit command
  • [+ ] I have added tests that prove my fix is effective or that my feature works
  • [n/a] I have added necessary documentation (if appropriate)

dkorobtsov added some commits Jan 29, 2018

Bug fix for occasional NPE's when SelenideReport used with TestNG fra…
…mework.

On certain occasions - when SimpleReport.start() method was not called, SelenideReport.finish() was causing NPE.

@asolntsev asolntsev self-requested a review Jan 29, 2018

@asolntsev asolntsev added this to the 4.11 milestone Jan 29, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Jan 29, 2018

Codecov Report

Merging #686 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #686      +/-   ##
============================================
+ Coverage     61.19%   61.24%   +0.04%     
- Complexity      797      798       +1     
============================================
  Files           152      152              
  Lines          2817     2820       +3     
  Branches        271      272       +1     
============================================
+ Hits           1724     1727       +3     
  Misses          987      987              
  Partials        106      106
Impacted Files Coverage Δ Complexity Δ
...com/codeborne/selenide/logevents/SimpleReport.java 100% <100%> (ø) 9 <0> (+1) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c409fa9...2048a87. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Jan 29, 2018

Coverage Status

Coverage increased (+0.04%) to 65.0% when pulling 2048a87 on dkorobtsov:bugfix-simplereport-npe into c409fa9 on codeborne:master.

@asolntsev asolntsev requested a review from vinogradoff Jan 29, 2018

@asolntsev asolntsev self-assigned this Jan 29, 2018

@@ -21,6 +21,11 @@ public void start() {
public void finish(String title) {
EventsCollector logEventListener = SelenideLogger.removeListener("simpleReport");
if (logEventListener == null) {

This comment has been minimized.

@vinogradoff

vinogradoff Jan 29, 2018

Member

it is actually a hack. It certainly breaks nothing, but actual question is, why/when does removeListener returns null?
Root cause is not addressed with this fix....
is the problem reproducible somehow?

This comment has been minimized.

@dkorobtsov

dkorobtsov Jan 30, 2018

Contributor

Agreed. You are right, it does not solve root cause. Made some research, it feels like in certain circumstances we have conflict between log4j2 and SelenideLogger. At least, I was able to reproduce the issue only in case when another logging library was used at the same time.

Here is a gist with steps, screenshots and code snippets to reproduce the issue:
https://gist.github.com/dkorobtsov/0d121bcb11a3c421e0dbc7ea93f0c8bc

This comment has been minimized.

@BorisOsipov

BorisOsipov Feb 1, 2018

Member

@vinogradoff I don't want to have NPE in Selenide code in any cases of user mistakes.

This comment has been minimized.

@dkorobtsov

dkorobtsov Feb 1, 2018

Contributor

@BorisOsipov @vinogradoff btw, just in case I've updated gist with another code sample and added some comments.

TLDR: context is null in case test was removed from suite scope for any reason on before invocation event before SelenideLogger.start() was called. Since afterInvocantion is called anyway, finish() is called with null context. In theory cheap and fast solution for this particular and all similar situations is to add boolean flag like "selenideLoggingStarted" - so finish() event will be called only in case logging has actually started.

@asolntsev asolntsev merged commit 0b37850 into selenide:master Mar 13, 2018

4 checks passed

codecov/patch 100% of diff hit (target 61.19%)
Details
codecov/project 61.24% (+0.04%) compared to c409fa9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 65.0%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment