Skip to content
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.

Sign up for GitHub

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

IDEA-113632 - Fix Geb Page content not being available #2862

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

IDEA-113632 - Fix Geb Page content not being available #2862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions plugins/groovy/groovy-psi/src/org/jetbrains/plugins/groovy/geb/GebUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,29 @@
package org.jetbrains.plugins.groovy.geb;

import com.intellij.psi.*;
import com.intellij.psi.impl.source.PsiImmediateClassType;
import com.intellij.psi.scope.PsiScopeProcessor;
import com.intellij.psi.util.CachedValueProvider.Result;
import com.intellij.psi.util.CachedValuesManager;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
import org.jetbrains.plugins.groovy.lang.psi.api.auxiliary.modifiers.GrModifierFlags;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrField;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrLabeledStatement;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.GrVariableDeclaration;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.blocks.GrClosableBlock;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrCall;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrExpression;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrMethodCall;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.expressions.GrReferenceExpression;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.params.GrParameter;
import org.jetbrains.plugins.groovy.lang.psi.api.statements.typedef.members.GrMethod;
import org.jetbrains.plugins.groovy.lang.psi.impl.statements.expressions.ClassUtil;
import org.jetbrains.plugins.groovy.lang.psi.impl.statements.expressions.GrReferenceExpressionImpl;
import org.jetbrains.plugins.groovy.lang.psi.impl.synthetic.GrLightField;
import org.jetbrains.plugins.groovy.lang.psi.impl.synthetic.GrLightMethodBuilder;
import org.jetbrains.plugins.groovy.lang.psi.util.PsiUtil;
import org.jetbrains.plugins.groovy.lang.resolve.ResolveUtil;

import java.util.Collections;
import java.util.HashMap;
Expand All @@ -37,12 +45,81 @@ public static boolean contributeMembersInsideTest(PsiScopeProcessor processor,

if (pageClass != null) {
if (!pageClass.processDeclarations(processor, state, null, place)) return false;
contributePageContent(processor, state, place);
}
}

return true;
}

private static void contributePageContent(PsiScopeProcessor processor, ResolveState state, PsiElement place) {
if(place instanceof GrReferenceExpressionImpl expr
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, why not just place instanceof GrReferenceExpression?

&& !(expr.getParent() instanceof GrMethodCall)) {

PsiClassType gebPageType = PsiType.getTypeByName("geb.Page", place.getProject(), place.getResolveScope());
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have PsiClass for geb.Page in GebUtil#contributeMembersInsideTest. Could you, please, create type from it by using TypesUtils#createType to avoid fq name dublication

PsiClass ourPage = findPageChange(expr, gebPageType);
if (ourPage != null) {
Map<String, PsiClass> supers = ClassUtil.getSuperClassesWithCache(ourPage);
String nameHint = ResolveUtil.getNameHint(processor);

for (PsiClass psiClass : supers.values()) {
Map<String, PsiMember> contentElements = getContentElements(psiClass);

if (nameHint == null) {
for (Map.Entry<String, PsiMember> entry : contentElements.entrySet()) {
processor.execute(entry.getValue(), state);
}
return;
}
else {
PsiVariable defElement = (PsiVariable)contentElements.get(nameHint);
if (defElement != null) {
processor.execute(defElement, state);
}
}
}
}
}
}


private static @Nullable PsiClass findPageChange(PsiElement place, PsiClassType pageType) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, add @Nullable/@NotNull to the parameters

PsiElement currentElement = place;
PsiClass ourPage = null;
while (currentElement != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that this method will be invoked for each reference inside the method, but you actually traverse the whole method starting from the innermost block to the outermost. Could you, please, rewrite it in the way like:

GrCodeBlock block = PsiTreeUtil.getParentOfType(place, GrCodeBlock.class):
for (var statement : block.getStatements()) {
   ...
}

PsiElement examineThis = currentElement;
Comment on lines +88 to +90
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, rename ourPage and examineThis, I doubt, if it is descriptive. Usually, we name inheritors of PsiElement according to their types.

if (currentElement instanceof GrLabeledStatement labeled) {
examineThis = labeled.getStatement();
}
if(examineThis instanceof GrVariableDeclaration) {
PsiElement findCall = examineThis.getLastChild();
while(findCall != null) {
if(findCall instanceof GrExpression) {
examineThis = findCall;
break;
}
findCall = findCall.getLastChild();
}
}
Comment on lines +95 to +103
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, This construction looks really strange to me. If I understood correctly, you are trying to retrieve the initializer expression of the last variable. To achieve such behaviour, there is more concise approach. You need to retrieve variables by GrVariableDeclaration#getVariables. And from there you can get GrVariable#getInitializerGroovy. Also, I don't understand why you check only last variable, although there can be multiple in the same line.

if (examineThis instanceof GrExpression call && !(examineThis instanceof GrReferenceExpression)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, replace to examineThis instanceof GrMethodCall

PsiType ret = call.getType();
if (ret != null && pageType.isAssignableFrom(ret) && ret instanceof PsiImmediateClassType ct) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I think that check ret instanceof PsiImmediateClassType ct is exhaustive, because you already know that your return type is assignable to the geb.Page

ourPage = ct.resolve();
break;
}
}
currentElement = currentElement.getPrevSibling();
}

if (ourPage == null) {
PsiElement parent = place.getParent();
if (parent != null && !(parent instanceof GrMethod))
return findPageChange(parent, pageType);
}

return ourPage;
}

public static Map<String, PsiMember> getContentElements(@NotNull PsiClass pageOrModuleClass) {
return CachedValuesManager.getCachedValue(pageOrModuleClass, () -> Result.create(
calculateContentElements(pageOrModuleClass), pageOrModuleClass
Expand Down
156 changes: 155 additions & 1 deletion plugins/groovy/test/org/jetbrains/plugins/groovy/geb/GebTestsTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import static org.jetbrains.plugins.groovy.GroovyProjectDescriptors.LIB_GROOVY_1
class GebTestsTest extends LightJavaCodeInsightFixtureTestCase {

private static final TestLibrary LIB_GEB = new RepositoryTestLibrary(
'org.codehaus.geb:geb-core:0.7.2',
'org.gebish:geb-core:7.0',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, use the version of library, when the feature was firstly available, I think it should be org.gebish:geb-core:0.9.0. Also, can you update to the same the other dependencies to the same version?

'org.codehaus.geb:geb-junit4:0.7.2',
'org.codehaus.geb:geb-spock:0.7.2',
'org.codehaus.geb:geb-testng:0.7.2'
Expand Down Expand Up @@ -283,4 +283,158 @@ class A extends geb.Page {
}
""")
}

void testPageContentIsInScope() {
myFixture.enableInspections(GroovyAssignabilityCheckInspection)

myFixture.configureByText("SomeSpec.groovy", """
class SomeSpec extends geb.spock.GebSpec {
Comment on lines +288 to +291
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, don't write tests using groovy language, because we have completely removed it from codebase recently

void "test"() {
expect:
to PageOne
geb.navigator.Navigator aNavigator = header
String aString = headerText
Integer <warning descr="Cannot assign 'Navigator' to 'Integer'">notAnInteger</warning> = header

then:
header
headerText
}
}

class PageOne extends geb.Page {
static content = {
header { \$("h1") }
headerText { header.text() }
}
}

""")

myFixture.checkHighlighting(true, false, true)
TestUtils.checkResolve(myFixture.file)
}

void testPageContentIsCompletable() {
myFixture.enableInspections(GroovyAssignabilityCheckInspection)

myFixture.configureByText("SomeSpec.groovy", """
class SomeSpec extends geb.spock.GebSpec {
void "test"() {
when:
to PageOne

then:
<caret>
}
}

class PageOne extends geb.Page {
static content = {
header { \$("h1") }
headerText { header.text() }
}
}

""")

TestUtils.checkCompletionType(myFixture, "header", "geb.navigator.Navigator")
TestUtils.checkCompletionType(myFixture, "headerText", "java.lang.String")
}

void testOnlyCurrentPageContentIsInScope() {
myFixture.enableInspections(GroovyAssignabilityCheckInspection)

myFixture.configureByText("SomeSpec.groovy", """
class SomeSpec extends geb.spock.GebSpec {
void "test"() {
when:
to PageOne

then:
header

when:
to PageTwo

then:
pageTwoContent
headerText
}
}

class PageOne extends geb.Page {
static content = {
header { \$("h1") }
headerText { header.text() }
}
}

class PageTwo extends geb.Page {
static content = {
pageTwoContent { \$("h1") }
}
}

""")

myFixture.checkHighlighting(true, false, true)
TestUtils.checkResolve(myFixture.file, "headerText")
}

void testChangingCurrentPage() {
myFixture.enableInspections(GroovyAssignabilityCheckInspection)

myFixture.configureByText("SomeSpec.groovy", """
class SomeSpec extends geb.spock.GebSpec {
void "to"() {
when:
to PageOne

then:
header
}

void "via"() {
when:
via PageOne

then:
header
}

void "at"() {
expect:
at PageOne
header
}

void "explicit"() {
when:
page(PageOne)

then:
header
}

void "assignment"() {
when:
geb.Page newPage = to PageOne

then:
header
}
}

class PageOne extends geb.Page {
static content = {
header { \$("h1") }
headerText { header.text() }
}
}
""")

myFixture.checkHighlighting(true, false, true)
TestUtils.checkResolve(myFixture.file)
}
}