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

chore: Run JDK9Test compiling to validate syntax. #1050

Merged
merged 2 commits into from
Jan 27, 2024

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Jan 26, 2024

See #958 - should improve the testing of the doc oriented test code

@He-Pin He-Pin marked this pull request as draft January 26, 2024 10:03
@He-Pin
Copy link
Member Author

He-Pin commented Jan 26, 2024

@Roiocam I just checked, the paradox task still can't catch the compiling error, but it does when running it locally.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 26, 2024

@mdedetrich @pjfanning @Roiocam
I think this should be the simplest way.
With an explicit TestJDK9 / test as I chatted with @Roiocam on WeChat.
https://github.com/apache/incubator-pekko/actions/runs/7669498804/job/20903555947?pr=1050#step:5:688

I tried to rewrite the plugin which just replaced the Test / unmanagedSourceDirectieries, but it will need to change more code.

@Roiocam
Copy link
Member

Roiocam commented Jan 26, 2024

clean;docs/test works for me too, I think we need to figure out why validatePullRequest rather than adding a CI check, what do you think? @He-Pin

[info] compiling 2 Scala sources and 2 Java sources to /Users/roiocam/IdeaProject/incubator-pekko/docs/target/scala-2.13/TestJdk9-classes ...
[error] /Users/roiocam/IdeaProject/incubator-pekko/docs/src/test/java-jdk9-only/jdocs/stream/operators/source/AsSubscriber.java:26:1: illegal start of type declaration
[error] public interface AsSubscriber {
[error] ^
[error] one error found
[info] Run completed in 1 minute, 18 seconds.
[info] Total number of tests run: 629
[info] Suites: completed 181, aborted 0
[info] Tests: succeeded 629, failed 0, canceled 0, ignored 6, pending 1
[info] All tests passed.
[error] (docs / TestJdk9 / compileIncremental) Compilation failed
[error] Total time: 349 s (05:49), completed 2024年1月27日 上午2:19:19

@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

@Roiocam Let's me update the pr and check if that works on ci too.

@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

I verified both works @Roiocam

@He-Pin He-Pin marked this pull request as ready for review January 27, 2024 09:39
@He-Pin He-Pin added this to the 1.1.0-M1 milestone Jan 27, 2024
@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

image

@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

It can now check the error code in Java 9 folder, and for Java 21 I will do in another PR. @mdedetrich @pjfanning

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

how does this prove the class has compiled? It doesn't use the class

@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

@pjfanning I mean the file is been compiled in the doc. Check the JDK9+ classes generated is not a target of this PR. Why do you always like to block a PR? you can just leave a comment.

@He-Pin He-Pin changed the title chore:test paradox failing chore: Run JDK9Test compiling to validate syntax. Jan 27, 2024
@pjfanning
Copy link
Contributor

@pjfanning I mean the file is been compiled in the doc. Check the JDK9+ classes generated is not a target of this PR. Why do you always like to block a PR? you can just leave a comment.

#1039 isn't even fixed yet - what we really need is to fix #1039 and then come up with a CI check that stops #1039 ever happening again

@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

@pjfanning I'm preparing a PR for #1039, separately.

@pjfanning
Copy link
Contributor

Motivation:
Fix #1040

I don't understand this PR at all. #1040 relates to #1039. How is this a fix for #1040?

@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

@pjfanning Because you forgot #958

@pjfanning
Copy link
Contributor

@pjfanning Because you forgot #958

It is the duty of the PR writer to explain their PR. You made no mention of #958 and claimed this is to be a fix for #1040 - which it isn't.

Can you now explain how this change helps with #958?

@He-Pin
Copy link
Member Author

He-Pin commented Jan 27, 2024

image

@pjfanning Now it will check the error. as ↑

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm but I'm going to remove the mention of #1040

@Roiocam
Copy link
Member

Roiocam commented Jan 27, 2024

@pjfanning @He-Pin

I have created an issue tracker for those build problems: #1052

I think this issue only related to #959

And #1039 has been fixed by #1047

Copy link
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for solving this, It should solve #959.

Leave a last picky suggestion.

.github/workflows/generate-doc-check.yml Outdated Show resolved Hide resolved
@Roiocam Roiocam mentioned this pull request Jan 27, 2024
32 tasks
Co-authored-by: AndyChen <chinatc@outlook.com>
Copy link
Member

@Roiocam Roiocam left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@He-Pin He-Pin merged commit a2b5b5d into apache:main Jan 27, 2024
18 checks passed
@He-Pin He-Pin deleted the paradoxTest branch January 27, 2024 14:14
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.

3 participants