Skip to content

Conversation

knysh
Copy link
Contributor

@knysh knysh commented Jan 30, 2020

PR Details

Related Issue Link:

#11

How Has This Been Tested

tests.configurations.EnvConfigurationTests
tests.configurations.ProfileConfigurationTests
tests.configurations.LoggerConfigurationTests
tests.configurations.TimeoutConfigurationTests
tests.configurations.RetryConfigurationTests
tests.configurations.ElementCacheConfigurationTests

Checklist
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

knysh added 28 commits January 27, 2020 19:23
updated com.fasterxml.jackson.core to 2.10.2
…Configuration

# Conflicts:
#	src/test/java/tests/configurations/EnvConfigurationTests.java
…Configuration

# Conflicts:
#	src/test/java/tests/configurations/EnvConfigurationTests.java
# Conflicts:
#	src/main/java/aquality/selenium/core/application/AqualityModule.java
#	src/main/java/aquality/selenium/core/application/AqualityServices.java
#	src/test/java/tests/logger/LoggerTests.java
…erConfiguration

# Conflicts:
#	src/main/java/aquality/selenium/core/application/AqualityModule.java
#	src/test/resources/TestSuite.xml
…Configuration

# Conflicts:
#	src/test/java/tests/configurations/EnvConfigurationTests.java
…Configuration

# Conflicts:
#	src/main/java/aquality/selenium/core/application/AqualityModule.java
#	src/test/java/tests/configurations/EnvConfigurationTests.java
# Conflicts:
#	src/main/java/aquality/selenium/core/application/AqualityModule.java
#	src/test/java/tests/utilities/CustomSettingsFileTests.java
#	src/test/java/tests/utilities/SettingsFileTests.java
#	src/test/resources/TestSuite.xml
#	src/test/resources/settings.json
#	src/test/resources/settings.local.json
@knysh knysh requested review from arazantsau and paveliam January 30, 2020 09:49
@@ -0,0 +1,5 @@
package aquality.selenium.core.localization;

public enum SupportedLanguage {
Copy link
Contributor

Choose a reason for hiding this comment

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

we would not need this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated


@Test
public void testShouldBePossibleToGetLanguageFromNewSettingsFile() {

Copy link
Contributor

Choose a reason for hiding this comment

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

redundant empty 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.

fixed

}

@AfterMethod
public void after() {
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure that this overriding of base class's method would work as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

public interface ILoggerConfiguration {

/**
* Gets language of the library.
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe 'of the logger' instead of 'of the library.'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


public class ElementCacheConfiguration implements IElementCacheConfiguration{

private static final String JSON_PATH = "/elementCache/isEnabled";
Copy link
Collaborator

Choose a reason for hiding this comment

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

could you please rename as IS_ENABLED_PATH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

private SettingsFileUtil() {
}

public static Object getValueOrDefault(ISettingsFile settingsFile, String path, Object defaultValue) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we move this in the default method of interface to remove this utility class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

getNumberAction.run();
} catch (Exception e) {
Assert.assertSame(e.getCause().getClass(), NumberFormatException.class);
Assert.assertTrue(e.getMessage().contains("NumberFormatException"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it necessary after first verification on exception type?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure if nedded second assert. Type of exception should be NumberFormatException

}

private long getTimeout(TIMEOUT timeout){
return Long.valueOf(settingsFile.getValue("/timeouts/" + timeout.getKey()).toString());
Copy link
Collaborator

Choose a reason for hiding this comment

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

would not it be better to move "/timeouts/" in the enumeration to keep full jsonpath in the one place? and getKey can be renamed to getPath or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@knysh knysh merged commit f3b0786 into master Jan 30, 2020
@knysh knysh deleted the Feature/11-IRetryConfiguration branch January 30, 2020 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants