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.

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

Fix CompletionContextTests0123 inside JavacConverter #1109

Merged

Conversation

robstryker
Copy link

This is a proof of concept of adding a specific erroneous / invalid node to the dom tree in all cases, not just completion, and seeing what regressions we get.

@mickaelistria
Copy link

This seems to fix 2 tests.

@robstryker
Copy link
Author

#1110 had no discernably different result.

Using the following script:

#!/bin/sh

curl -H 'Cache-Control: no-cache' --compressed https://ci.eclipse.org/ls/job/jdt-core-incubator/job/PR-$1/lastSuccessfulBuild/testReport/api/xml | xmllint --xpath '/testResult/suite/case[status != "PASSED"]/*[name() = "className" or name() = "name"]/text()' - | sed 'N; s/\n/ /' - | sort > /home/rob/scripts/jdt_pr_data/pr.txt

curl -H 'Cache-Control: no-cache' --compressed https://ci.eclipse.org/ls/job/jdt-core-incubator/job/dom-with-javac/lastSuccessfulBuild/testReport/api/xml | xmllint --xpath '/testResult/suite/case[status != "PASSED"]/*[name() = "className" or name() = "name"]/text()' - | sed 'N; s/\n/ /' - | sort > /home/rob/scripts/jdt_pr_data/baseline.txt

diff /home/rob/scripts/jdt_pr_data/pr.txt /home/rob/scripts/jdt_pr_data/baseline.txt

wc -l  ~/scripts/jdt_pr_data/baseline.txt
wc -l  ~/scripts/jdt_pr_data/pr.txt

I noticed the following output:

2754 /home/rob/scripts/jdt_pr_data/baseline.txt
2687 /home/rob/scripts/jdt_pr_data/pr.txt

@robstryker
Copy link
Author

Nevermind. My script didn't account for "FIXED" status.

Yes, fixes 2. Looks good to me. Let me know if I should merge.

@mickaelistria
Copy link

Feel free to merge it ASAP.

int lineEnd = newLine == -1 ? this.rawText.length() - 1 : newLine;
String litText = this.rawText.substring(pos+1, lineEnd);
Expression res = convertStringToLiteral(litText, pos, lineEnd, null);
res.setSourceRange(pos, lineEnd - pos);

Choose a reason for hiding this comment

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

Maybe add something like res.setFlags(res.getFlags() | ASTNode.MALFORMED)

@robstryker robstryker force-pushed the dom-with-javac branch 3 times, most recently from 9c17bb6 to 4516c31 Compare January 22, 2025 21:22
@datho7561 datho7561 force-pushed the dom-with-javac branch 2 times, most recently from a1bf22f to 0b6fd1c Compare January 27, 2025 15:37
@robstryker robstryker force-pushed the dom-with-javac branch 2 times, most recently from 838c386 to 40594bf Compare January 31, 2025 18:15
@mickaelistria mickaelistria force-pushed the dom-with-javac branch 2 times, most recently from c906abc to b53a413 Compare February 7, 2025 10:49
@robstryker robstryker force-pushed the dom-with-javac branch 2 times, most recently from 08ec5cf to 10fe71a Compare February 7, 2025 20:43
@mickaelistria mickaelistria force-pushed the dom-with-javac branch 2 times, most recently from c2b9da2 to c640bdc Compare February 10, 2025 10:41
Rob Stryker added 2 commits February 12, 2025 12:28
Signed-off-by: Rob Stryker <stryker@redhat.com>
Signed-off-by: Rob Stryker <stryker@redhat.com>
@robstryker robstryker merged commit aa5cb79 into eclipse-jdtls:dom-with-javac Feb 12, 2025
4 of 6 checks passed
@robstryker
Copy link
Author

This broke dom-with-javac somehow. Looking into it.

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.

2 participants