Sonarcube violations fixed. #1481

krmahadevan · 2017-07-19T03:36:02Z

Below violations were fixed:

Reorder modifiers to comply with JLS.
Convert field into local variable.
Remove usage of synchronized class Vector and replace with non-threadsafe/threadsafe alternative.
Remove un-used imports.
Remove redundant array creation when translating to List
Refactor map initialisation to a static block/constructor
Return empty array instead of null.
Use Integer.toString() instead of string concatenation.
Use %n instead of \n to produce platform-specific line separator

Fixes # .

Did you remember to?

Add test case(s)
Update CHANGES.txt

We encourage pull requests that:

Add new features to TestNG (or)
Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

krmahadevan · 2017-07-19T03:36:32Z

ping @juherr

juherr

Few minor changes and some open points.

juherr · 2017-07-19T20:08:16Z

src/main/java/org/testng/internal/PackageUtils.java

@@ -31,7 +32,7 @@
  private static String[] testClassPaths;

  /** The additional class loaders to find classes in. */
-  private static final List<ClassLoader> classLoaders = new Vector<>();
+  private static final Collection<ClassLoader> classLoaders = new ConcurrentLinkedDeque<>();


Do we really need a concurrent collection? Why not just ArrayList?

I am not sure about the intent behind having this collection as a concurrent one. The method that directly adds elements into this collection is org.testng.internal.PackageUtils#addClassLoader and I don't see any direct invocations to this method from within TestNG. But that being said, I don't know if there are external customers (like the IntelliJ plugin or the surefire plugin or the eclipse plugin) which makes use of this method and how do they do it. If this method gets invoked from concurrent threads then there's a very good chance of triggering ConcurrentModificationException (if an ArrayList is used). That was why I was playing it safe by making use of a concurrent collection (In short adhering to the existing codebase, but just replacing Vector with a concurrent collection.

juherr · 2017-07-19T20:10:08Z

src/main/java/org/testng/internal/Parameters.java


  }

+  private static List<Class<?>> newList(Class<?>...elements) {


I'm not sure for that:

List<Class<?>> ctxTest = Arrays.asList(ITestContext.class, XmlTest.class); mapping.put(Test.class.getSimpleName(), Collections.singletonList(ITestContext.class));

instead?

This won't work because the Arrays.asList(ITestContext.class, XmlTest.class); when invoked results in a List<Class<? extends Serializable> (both ITestResult and XmlTest implement Serializable) and so Java deduces this type. I tried this already and since it wasn't working is why I resorted to this approach.

And what about

List<Class<?>> ctxTest = Arrays.<Class<?>>asList(ITestContext.class, XmlTest.class);

@juherr... nice ! I never knew we could use Arrays.asList() in that fashion wherein we specify explicitly the return type... (Guess its time for me to go back and read more about generics). Yes, that would work. But now that brings me back to the question...

Apart from the succinctness (which is the only thing that stands obvious to my naive eyes), how is what you suggested different from what I have as an implementation in newList() ? I am curious. Can you please tell me ?

It is totally debatable but I think we don't need a specific method to do a so simple thing which takes only 1 line.

juherr · 2017-07-19T20:12:36Z

src/main/java/org/testng/internal/Utils.java

   * @param sb the file content
   */
-  private static void writeFile(@Nullable File outDir, String fileName, String sb, @Nullable String encoding) {
+  private static void writeFile(@Nullable File outputFolder, String filename, String sb, @Nullable String encoding) {
+    File outDir = outputFolder;


I don't understand the value add of this rule.

The general rule is that method parameters are not supposed to be altered within a method but they should instead be copied to a different variable and then operated upon.

ok, but it's mostly a design issue somewhere or you need to find a better name for this attributes. Because variation on min/maj is error prone.

juherr · 2017-07-19T20:13:53Z

src/main/java/org/testng/reporters/SuiteHTMLReporter.java

@@ -40,6 +40,8 @@
  public static final String REPORTER_OUTPUT = "reporter-output.html";
  public static final String METHODS_NOT_RUN = "methods-not-run.html";
  public static final String TESTNG_XML = "testng.xml.html";
+  private static final String TD_A_TARGET_MAIN_FRAME_HREF = "<td><a target='mainFrame' href='";
+  private static final String TD = "</td>";


maybe END_TD or CLOSE_TD instead

Below violations were fixed: * Reorder modifiers to comply with JLS. * Convert field into local variable. * Remove usage of synchronized class Vector and replace with non-threadsafe/threadsafe alternative. * Remove un-used imports. * Remove redundant array creation when translating to List * Refactor map initialisation to a static block/constructor * Return empty array instead of null. * Use Integer.toString() instead of string concatenation. * Use %n instead of \n to produce platform-specific line separator

krmahadevan · 2017-07-21T03:02:04Z

@juherr - I fixed all the comments and pushed in the changes. Please let me know if this now good to be merged.

juherr

LGTM 👍
Ping @cbeust

juherr requested changes Jul 19, 2017

View reviewed changes

krmahadevan added 2 commits July 21, 2017 07:59

Fixing review comments.

a4a7562

krmahadevan force-pushed the krmahadevan-fix-sonarcube-issues branch from 6e8ba80 to a4a7562 Compare July 21, 2017 02:40

juherr approved these changes Jul 21, 2017

View reviewed changes

cbeust merged commit 971aca1 into testng-team:master Jul 22, 2017

krmahadevan deleted the krmahadevan-fix-sonarcube-issues branch July 22, 2017 17:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sonarcube violations fixed. #1481

Sonarcube violations fixed. #1481

krmahadevan commented Jul 19, 2017

krmahadevan commented Jul 19, 2017

juherr left a comment

juherr Jul 19, 2017

krmahadevan Jul 20, 2017

juherr Jul 20, 2017

juherr Jul 19, 2017

krmahadevan Jul 20, 2017 •

edited

Loading

juherr Jul 20, 2017

krmahadevan Jul 20, 2017

juherr Jul 20, 2017

juherr Jul 19, 2017

krmahadevan Jul 20, 2017

juherr Jul 20, 2017

juherr Jul 19, 2017

krmahadevan Jul 20, 2017

krmahadevan commented Jul 21, 2017

juherr left a comment


		}

		private static List<Class<?>> newList(Class<?>...elements) {

Sonarcube violations fixed. #1481

Sonarcube violations fixed. #1481

Conversation

krmahadevan commented Jul 19, 2017

Did you remember to?

krmahadevan commented Jul 19, 2017

juherr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmahadevan Jul 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krmahadevan commented Jul 21, 2017

juherr left a comment

Choose a reason for hiding this comment

krmahadevan Jul 20, 2017 •

edited

Loading