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 ESQL tests in IntelliJ #107313

Merged
merged 5 commits into from
Apr 11, 2024
Merged

Conversation

nik9000
Copy link
Member

@nik9000 nik9000 commented Apr 10, 2024

Landing #107018 broke running ESQL unit tests in IntelliJ. It has something to do with turning on the stringtemplate plugin in the esql project but I don't really know what. After that PR we'd often get errors about trying to regenerate evaluators twice. I dunno. This fixes it. But I don't really know why.

The way this fixes it is by making the esql project more like the copmute project. It makes sense that that would help - they both have the same code generation configuration. Anyway, the operative change is landing the generated files in the same place as the compute project. Thus all of the file moves.

Again, I have no idea why this works. It's build black magic and I just shook it until it worked. Most of the credit goes to git-bisect for finding the commit that broke this.

Landing elastic#107018 broke running ESQL unit tests in IntelliJ. It has
*something* to do with turning on the stringtemplate plugin in the esql
project but I don't really know what. After that PR we'd often get
errors about trying to regenerate evaluators twice. I dunno. This fixes
it. But I don't really know why.

The way this fixes it is by making the `esql` project more like the
`copmute` project. It makes sense that that would help - they both have
the same code generation configuration. Anyway, the operative change is
landing the generated files in the same place as the `compute` project.
Thus all of the file moves.

Again, I have no idea why this works. It's build black magic and I just
shook it until it worked. Most of the credit goes to git-bisect for
finding the commit that broke this.
@nik9000 nik9000 added >test Issues or PRs that are addressing/adding tests :Analytics/ES|QL AKA ESQL v8.14.0 labels Apr 10, 2024
@nik9000 nik9000 requested a review from a team as a code owner April 10, 2024 15:00
@elasticsearchmachine elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 10, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-analytical-engine (Team:Analytics)

tasks.named("compileJava").configure {
options.compilerArgs.addAll(["-s", "${projectDir}/src/main/java/generated"])
Copy link
Member Author

Choose a reason for hiding this comment

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

It's now making them in src/main/generated-src/generated and I can't move it. It looks like this isn't configurable in intellij. But the compute subproject does the right thing and intellij generates the classes to src/main/generated. I have no idea what's up. But this works around it.

If any expert in gradle has time to understand this, great! You could fix this a lot better than I can.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. This fails in gradle now...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. This seems to work.

Copy link
Contributor

@alex-spies alex-spies left a comment

Choose a reason for hiding this comment

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

Gave it a look and looks okay to me, but agree that it'd be great if @elastic/es-delivery could check if this can be fixed better.

While we're touching the generated src logic, maybe we should also remove the obsolete line from the top-level .gitignore? It's x-pack/plugin/esql/gen/ and that directory doesn't even exist anymore.

Comment on lines +45 to +46
// IntelliJ sticks generated files here and we can't stop it....
exclude { it.file.toString().startsWith("${projectDir}/src/main/generated-src/generated") }
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this is annoying of IntelliJ :/

@craigtaverner
Copy link
Contributor

I've cherry-picked these commits into my branch to work around this issue, and can report that it works but for one strange thing. The versions of the evaluators with incorrect formatting are repeatedly being created (not sure if by gradle or intellij, since I'm running both) and I have to regularly run spotlessApply to get the correct versions back.

@nik9000
Copy link
Member Author

nik9000 commented Apr 11, 2024

I can't reproduce @craigtaverner's problem. I spent a while on video with him and we've tracked it down to something to do with his branch. If he switches to my branch it's fine. But that's where we stopped. We'll dig more soon.

Copy link
Contributor

@bpintea bpintea left a comment

Choose a reason for hiding this comment

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

fwiw, it lgtm

@nik9000
Copy link
Member Author

nik9000 commented Apr 11, 2024

So that I can use this I'm going to merge it. I expect someone from delivery land to come and tell me the right way to do this soon and will happily do the gradle the way they prefer. Or, maybe, wondrously, they can figure out how to configure intellij the right way.

@nik9000 nik9000 merged commit 2c0e90f into elastic:main Apr 11, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/ES|QL AKA ESQL Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >test Issues or PRs that are addressing/adding tests v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants