Feature/#12 stabilization of login modifier#28
Conversation
|
|
||
| import java.util.Map; | ||
|
|
||
| public class LoginModifierConfig { |
There was a problem hiding this comment.
Could we have this class package-scoped?
| form.login(config.getLogin(), config.getPassword()); | ||
| } | ||
|
|
||
| private void sleep() { |
There was a problem hiding this comment.
Could we rename this method to sth like:
waitForAuthenticationdelayBeforeLoginCheck- ??
There was a problem hiding this comment.
Renamed to delayBeforeLoginCheckOrReattempt. Long, but descriptive - this timeout is used in two contexts.
| this.loginTokenKey = getParameter(params, LOGIN_TOKEN_KEY_PARAM, DEFAULT_LOGIN_TOKEN); | ||
| this.forceLogin = BooleanUtils.toBoolean(params.get(FORCE_LOGIN)); | ||
| int loginTimeout = NumberUtils.toInt(getParameter(params, LOGIN_CHECK_TIMEOUT_PARAM, DEFAULT_CHECK_TIMEOUT_PARAM)); | ||
| this.loginCheckTimeout = Math.min(10000, loginTimeout); |
There was a problem hiding this comment.
Could we rename this attribute to sth like:
this.waitForAuthentication = loginCheckDelaythis.delayBeforeLoginCheck = delay- ??
There was a problem hiding this comment.
Renamed to delayBeforeLoginCheckOrReattempt. Long, but descriptive - this timeout is used in two contexts.
| Thread.sleep(config.getLoginCheckTimeout()); | ||
| } catch (InterruptedException e) { | ||
| LOGGER.error("Interruption", e); | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
Hmm. This line looks suspicious.
I went through one article about the topic:
http://www.javaspecialists.eu/archive/Issue056.html
and it looks like it makes sense for interrupting newly created threads.
Here however we are in some thread that is performing screen collection.
I woud rather throw some runtime exception here:
new RuntimeException("this thread should NOT be interrupted here")..
See also https://docs.oracle.com/javase/7/docs/api/java/lang/Thread.html#interrupt()
Could we consult someone on this?
There was a problem hiding this comment.
I think this is so little damage here when sleep is interrupted, we can go with this approach. It looks that SleepModifier handles interruption in similar way - it may be area to improve at least logging there.
There was a problem hiding this comment.
Left as it is.
| private LoginModifierConfig config; | ||
|
|
||
| LoginModifier(WebCommunicationWrapper webCommunicationWrapper, ValidationResultBuilder validationResultBuilder) { | ||
| this.webCommunicationWrapper = webCommunicationWrapper; |
There was a problem hiding this comment.
Could we also have this.webDriver in line 49 ?
| <profiles> | ||
| <profile> | ||
| <!-- Assembly test suites from 'partials' catolog into one main-test.xml --> | ||
| <!-- Assembly test suites from 'partials' catalog into one main-test.xml --> |
There was a problem hiding this comment.
Please avoid changes with only white-space characters modification.
There was a problem hiding this comment.
Actually it's a typo fix: catolog -> catalog
| } | ||
|
|
||
| private void sleep() { | ||
| if (config.getLoginCheckTimeout() > 0) { |
There was a problem hiding this comment.
We use sleep in two places. Its action depends on LoginCheckTimeout value. Should sleep method be used both before each re-attempt of login and during login method?
There was a problem hiding this comment.
Yes. I'm going to describe it in the documentation.
| Thread.sleep(config.getLoginCheckTimeout()); | ||
| } catch (InterruptedException e) { | ||
| LOGGER.error("Interruption", e); | ||
| Thread.currentThread().interrupt(); |
There was a problem hiding this comment.
I think this is so little damage here when sleep is interrupted, we can go with this approach. It looks that SleepModifier handles interruption in similar way - it may be area to improve at least logging there.
|
Please update Login Modifier Documentation in this pull request. |
4 participants
No description provided.