sonatype-lift[bot] commented on code in PR #1747:
URL: https://github.com/apache/groovy/pull/1747#discussion_r922570423
##########
src/test/org/codehaus/groovy/classgen/asm/sc/FieldsAndPropertiesStaticCompileTest.groovy:
##########
@@ -759,33 +815,34 @@ import
org.codehaus.groovy.transform.sc.ListOfExpressionsExpression
new A().test()
'''
} finally {
- assert !astTrees['A'][1].contains('pfaccess$00') // no mutator
bridge method for 'accessed'
- assert !astTrees['A'][1].contains('pfaccess$1') // no accessor
bridge method for 'mutated'
- assert astTrees['A$_closure1'][1].contains('INVOKESTATIC
A.pfaccess$2 (LA;)Ljava/lang/String;')
- assert astTrees['A$_closure1'][1].contains('INVOKESTATIC
A.pfaccess$02 (LA;Ljava/lang/String;)Ljava/lang/String;')
+ def dump = astTrees['A'][1]
+ assert dump.contains('pfaccess$0') // accessor bridge method for
'accessed'
+ assert !dump.contains('pfaccess$00') // no mutator bridge method
for 'accessed'
+ assert dump.contains('pfaccess$01') // mutator bridge method for
'mutated'
+ assert dump.contains('pfaccess$1') // accessor bridge method for
'mutated' -- GROOVY-9385
+ assert dump.contains('pfaccess$2') // accessor bridge method for
'accessedAndMutated'
+ assert dump.contains('pfaccess$02') // mutator bridge method for
'accessedAndMutated'
+ dump = astTrees['A$_closure1'][1]
+ assert dump.contains('INVOKESTATIC A.pfaccess$2
(LA;)Ljava/lang/String;')
Review Comment:
*AssertWithinFinallyBlock:* A finally block within class
org.codehaus.groovy.classgen.asm.sc.FieldsAndPropertiesStaticCompileTest
contains an assert statement, potentially hiding the original exception, if
there is one
Reply with *"**@sonatype-lift help**"* for info about LiftBot commands.
Reply with *"**@sonatype-lift ignore**"* to tell LiftBot to leave out the
above finding from this PR.
Reply with *"**@sonatype-lift ignoreall**"* to tell LiftBot to leave out all
the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to **refresh** the page to see its
response. [Click here](https://help.sonatype.com/lift/talking-to-lift) to get
to know more about LiftBot commands.
---
Was this a good recommendation?
[ [🙁 Not
relevant](https://www.sonatype.com/lift-comment-rating?comment=298936555&lift_comment_rating=1)
] - [ [😕 Won't
fix](https://www.sonatype.com/lift-comment-rating?comment=298936555&lift_comment_rating=2)
] - [ [😑 Not critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=298936555&lift_comment_rating=3)
] - [ [🙂 Critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=298936555&lift_comment_rating=4)
] - [ [😊 Critical, fixing
now](https://www.sonatype.com/lift-comment-rating?comment=298936555&lift_comment_rating=5)
]
##########
src/test/org/codehaus/groovy/control/customizers/ImportCustomizerTest.groovy:
##########
@@ -19,97 +19,160 @@
package org.codehaus.groovy.control.customizers
import org.codehaus.groovy.control.CompilerConfiguration
+import org.junit.Before
+import org.junit.Test
/**
- * Tests the import customizer.
+ * Tests for {@link ImportCustomizer}.
*/
-class ImportCustomizerTest extends GroovyTestCase {
- CompilerConfiguration configuration
- ImportCustomizer importCustomizer
+final class ImportCustomizerTest {
+ private final CompilerConfiguration configuration = new
CompilerConfiguration()
+ private final ImportCustomizer importCustomizer = new ImportCustomizer()
+
+ @Before
void setUp() {
- configuration = new CompilerConfiguration()
- importCustomizer = new ImportCustomizer()
configuration.addCompilationCustomizers(importCustomizer)
}
+ @Test
void testAddImport() {
-
importCustomizer.addImports("java.util.concurrent.atomic.AtomicInteger")
+
importCustomizer.addImports('java.util.concurrent.atomic.AtomicInteger')
def shell = new GroovyShell(configuration)
- shell.evaluate("new AtomicInteger(0)")
+ shell.evaluate('new AtomicInteger(0)')
// no exception means success
}
+ @Test
void testAddImportWithAlias() {
-
importCustomizer.addImport("AI","java.util.concurrent.atomic.AtomicInteger")
+ importCustomizer.addImport('AI',
'java.util.concurrent.atomic.AtomicInteger')
def shell = new GroovyShell(configuration)
- shell.evaluate("new AI(0)")
+ shell.evaluate('new AI(0)')
// no exception means success
}
+ @Test
void testAddInnerClassImport() {
-
importCustomizer.addImports("org.codehaus.groovy.control.customizers.ImportCustomizerTest.Inner")
+
importCustomizer.addImports('org.codehaus.groovy.control.customizers.ImportCustomizerTest.Inner')
def shell = new GroovyShell(configuration)
- shell.evaluate("new Inner()")
+ shell.evaluate('new Inner()')
// no exception means success
}
+ @Test
void testAddInnerClassImport2() {
-
importCustomizer.addImports("org.codehaus.groovy.control.customizers.ImportCustomizerTest")
+
importCustomizer.addImports('org.codehaus.groovy.control.customizers.ImportCustomizerTest')
def shell = new GroovyShell(configuration)
- shell.evaluate("new ImportCustomizerTest.Inner()")
+ shell.evaluate('new ImportCustomizerTest.Inner()')
// no exception means success
}
+ @Test
void testAddStaticImport() {
- importCustomizer.addStaticImport("java.lang.Math", "PI")
+ importCustomizer.addStaticImport('java.lang.Math', 'PI')
def shell = new GroovyShell(configuration)
- shell.evaluate("PI")
+ shell.evaluate('PI')
// no exception means success
}
+ @Test
void testAddStaticImportWithAlias() {
- importCustomizer.addStaticImport("pi","java.lang.Math", "PI")
+ importCustomizer.addStaticImport('pi', 'java.lang.Math', 'PI')
def shell = new GroovyShell(configuration)
- shell.evaluate("pi")
+ shell.evaluate('pi')
// no exception means success
}
+ @Test
void testAddStaticStarImport() {
- importCustomizer.addStaticStars("java.lang.Math")
+ importCustomizer.addStaticStars('java.lang.Math')
def shell = new GroovyShell(configuration)
- shell.evaluate("PI")
+ shell.evaluate('PI')
// no exception means success
}
+ @Test
void testAddStarImport() {
- importCustomizer.addStarImports("java.util.concurrent.atomic")
+ importCustomizer.addStarImports('java.util.concurrent.atomic')
def shell = new GroovyShell(configuration)
- shell.evaluate("new AtomicInteger(0)")
+ shell.evaluate('new AtomicInteger(0)')
// no exception means success
}
+ @Test
void testAddImports() {
-
importCustomizer.addImports("java.util.concurrent.atomic.AtomicInteger","java.util.concurrent.atomic.AtomicLong")
+
importCustomizer.addImports('java.util.concurrent.atomic.AtomicInteger',
'java.util.concurrent.atomic.AtomicLong')
def shell = new GroovyShell(configuration)
- shell.evaluate("""new AtomicInteger(0)
- new AtomicLong(0)""")
+ shell.evaluate('''
+ new AtomicInteger(0)
+ new AtomicLong(0)
+ ''')
// no exception means success
}
+ @Test
void testAddImportsOnScriptEngine() {
- File script = File.createTempFile('test', '.groovy')
- script.deleteOnExit()
- script.write """
+ importCustomizer.addImports('java.text.SimpleDateFormat')
+
+ def script = File.createTempFile('test', '.groovy')
+ script.deleteOnExit()
+ script.write '''
println new SimpleDateFormat()
- """
+ '''
+ // run script with script engine; this will not work if import
customizer is not used
+ def scriptEngine = new GroovyScriptEngine(script.parent)
+ scriptEngine.config = configuration
+ scriptEngine.run(script.name, new Binding())
+ }
- importCustomizer.addImports 'java.text.SimpleDateFormat'
+ @Test // GROOVY-8399
+ void testAddImportsOnModuleWithMultipleClasses() {
+ importCustomizer.addImports('java.text.SimpleDateFormat')
+ def shell = new GroovyShell(configuration)
+ shell.evaluate('''\
+ @groovy.transform.ASTTest(phase=SEMANTIC_ANALYSIS, value={
+ def imports = node.module.imports*.text
+ assert imports == ['import java.text.SimpleDateFormat as
SimpleDateFormat']
+ })
+ class A {
+ static class AA {
+ }
+ }
+ class B {
+ static class BB {
+ }
+ }
+ class C {
+ static class CC {
+ }
+ }
+ println new SimpleDateFormat()
+ ''')
+ }
- // Run script with script engine: this will not work, import
customizer is not used
- GroovyScriptEngine scriptEngine = new GroovyScriptEngine(script.parent)
- scriptEngine.setConfig configuration
- scriptEngine.run script.name, new Binding()
+ @Test // GROOVY-8399
+ void testAddStarImportsOnModuleWithMultipleClasses() {
+ importCustomizer.addStarImports('java.text', 'groovy.transform')
+ def shell = new GroovyShell(configuration)
+ shell.evaluate('''\
+ @groovy.transform.ASTTest(phase=SEMANTIC_ANALYSIS, value={
+ def imports = node.module.starImports*.text
+ assert imports == ['import java.text.*', 'import
groovy.transform.*']
+ })
+ class A {
+ static class AA {
+ }
+ }
+ class B {
+ static class BB {
+ }
+ }
+ class C {
+ static class CC {
+ }
+ }
+ println new SimpleDateFormat()
+ ''')
}
protected static class Inner {}
Review Comment:
*EmptyClass:* Class 'ImportCustomizerTest$Inner' is empty (has no methods,
fields or properties). Why would you need a class like this?
Reply with *"**@sonatype-lift help**"* for info about LiftBot commands.
Reply with *"**@sonatype-lift ignore**"* to tell LiftBot to leave out the
above finding from this PR.
Reply with *"**@sonatype-lift ignoreall**"* to tell LiftBot to leave out all
the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to **refresh** the page to see its
response. [Click here](https://help.sonatype.com/lift/talking-to-lift) to get
to know more about LiftBot commands.
---
Was this a good recommendation?
[ [🙁 Not
relevant](https://www.sonatype.com/lift-comment-rating?comment=298936578&lift_comment_rating=1)
] - [ [😕 Won't
fix](https://www.sonatype.com/lift-comment-rating?comment=298936578&lift_comment_rating=2)
] - [ [😑 Not critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=298936578&lift_comment_rating=3)
] - [ [🙂 Critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=298936578&lift_comment_rating=4)
] - [ [😊 Critical, fixing
now](https://www.sonatype.com/lift-comment-rating?comment=298936578&lift_comment_rating=5)
]
##########
src/test/org/codehaus/groovy/runtime/powerassert/AssertionRenderingTest.groovy:
##########
@@ -303,270 +328,331 @@ assert [(a):b, (c):d] == null
}
}
+ @Test
void testListExpression() {
- isRendered """
+ isRendered '''
assert [a,b,c] == null
| | | |
1 2 3 false
- """, {
+ ''', { ->
def a = 1
def b = 2
def c = 3
assert [a,b,c] == null
}
}
+ @Test
void testRangeExpression() {
- isRendered """
+ isRendered '''
assert (a..b) == null
| | |
1 2 false
- """, {
+ ''', { ->
def a = 1
def b = 2
assert (a..b) == null
}
- isRendered """
+ isRendered '''
assert (a..<b) == null
| | |
1 2 false
- """, {
+ ''', { ->
def a = 1
def b = 2
assert (a..<b) == null
}
}
+ @Test
void testPropertyExpression() {
- isRendered """
+ isRendered '''
assert a.bytes == null
| | |
| [65] false
'A'
-
- """, {
+ ''', { ->
def a = 'A'
assert a.bytes == null
}
- isRendered """
+ isRendered '''
assert Integer.MIN_VALUE == null
| |
| false
-2147483648
- """, {
+ ''', { ->
assert Integer.MIN_VALUE == null
}
}
+ @Test
void testAttributeExpression() {
- isRendered """
+ isRendered '''
assert holder.@x
| |
h 0
- """, {
+ ''', { ->
def holder = new Holder()
assert holder.@x
}
+
+ isRendered '''
+assert holder.@x != 0
+ | | |
+ h 0 false
+ ''', { ->
+ def holder = new Holder()
+ assert holder.@x != 0
+ }
+
+ isRendered '''
+assert 0 != holder.@x
+ | | |
+ | h 0
+ false
+ ''', { ->
+ def holder = new Holder()
+ assert 0 != holder.@x
+ }
+
+ isRendered '''
+assert this.@field == 0
+ | |
+ 1 false
+ ''', { ->
+ new Runnable() {
+ private int field = 1
+ @Override
+ public void run() {
+ assert this.@field == 0
+ }
+ }.run()
+ }
+
+ isRendered '''
+assert 0 == this.@field
+ | |
+ false 1
+ ''', { ->
+ new Runnable() {
+ private int field = 1
+ @Override
+ public void run() {
+ assert 0 == this.@field
+ }
+ }.run()
+ }
}
+ @Test
void testMethodPointerExpression() {
- isRendered """
+ isRendered '''
assert a.&"\$b" == null
| | |
[] | false
'get'
- """, {
+ ''', { ->
def a = []
def b = "get"
assert a.&"$b" == null
}
}
+ @Test
void testConstantExpression() {
- isRendered """
+ isRendered '''
assert 1 == "abc"
|
false
- """, {
+ ''', { ->
assert 1 == "abc"
Review Comment:
*ComparisonOfTwoConstants:* Comparing two constants or constant literals is
useless and may indicate a bug: (1 == abc)
Reply with *"**@sonatype-lift help**"* for info about LiftBot commands.
Reply with *"**@sonatype-lift ignore**"* to tell LiftBot to leave out the
above finding from this PR.
Reply with *"**@sonatype-lift ignoreall**"* to tell LiftBot to leave out all
the findings from this PR and from the status bar in Github.
When talking to LiftBot, you need to **refresh** the page to see its
response. [Click here](https://help.sonatype.com/lift/talking-to-lift) to get
to know more about LiftBot commands.
---
Was this a good recommendation?
[ [🙁 Not
relevant](https://www.sonatype.com/lift-comment-rating?comment=298936602&lift_comment_rating=1)
] - [ [😕 Won't
fix](https://www.sonatype.com/lift-comment-rating?comment=298936602&lift_comment_rating=2)
] - [ [😑 Not critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=298936602&lift_comment_rating=3)
] - [ [🙂 Critical, will
fix](https://www.sonatype.com/lift-comment-rating?comment=298936602&lift_comment_rating=4)
] - [ [😊 Critical, fixing
now](https://www.sonatype.com/lift-comment-rating?comment=298936602&lift_comment_rating=5)
]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
