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 local testing #27

Merged
merged 4 commits into from
Feb 7, 2024
Merged

Conversation

ericvergnaud
Copy link
Contributor

@ericvergnaud ericvergnaud commented Feb 7, 2024

@ftomassetti this works for me, can you check the same ?

(this PR doesn't fix the maven plugin tests, which would require help from @jimidle)

Copy link
Collaborator

@ftomassetti ftomassetti left a comment

Choose a reason for hiding this comment

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

Looks good to me (with that I mean that looking at the code, it looks good to me).
I will also try to run local tests and see what happens on my side.

@ftomassetti
Copy link
Collaborator

I tried to run the tests locally, I am not sure if the PR is expected to fix running tests, but just in case this is useful this is what I get.

I tried to:

  • clean the repo (git clean -fdX && mvn clean)
  • install mvn install -DskipTests=true -Darguments="-Dmaven.javadoc.skip=true" -B -V
  • run mvn test

This fails locally:

[INFO] ANTLR 5 ............................................ SUCCESS [  0.224 s]
[INFO] ANTLR 5 Runtime .................................... SUCCESS [  4.604 s]
[INFO] ANTLR 5 Kotlin Runtime ............................. SUCCESS [ 17.654 s]
[INFO] ANTLR 5 Tool ....................................... SUCCESS [  5.553 s]
[INFO] ANTLR 5 Maven plugin ............................... FAILURE [  5.269 s]
[INFO] ANTLR 5 Runtime Tests (4th generation) ............. SKIPPED
[INFO] ANTLR 5 Tool Tests ................................. SKIPPED

If I then move into runtime tests and run mvn test. I get some failues.

For example:

[ERROR] Tests run: 663, Failures: 28, Errors: 0, Skipped: 0, Time elapsed: 73.374 s <<< FAILURE! - in org.antlr.v5.test.runtime.java.api.TestExpectedTokens

If I then move into tool tests and run mvn test that succeeds.

@ericvergnaud
Copy link
Contributor Author

Thanks
We'll have to understand why the runtime tests fail locally when they succeed in the CI, but that's progress

@ericvergnaud
Copy link
Contributor Author

If I run:
mvn -Dtest='java.**' test
or
mvn -Dtest='kotlin.**' test

I suspect that mvn test fails because it also tries running ts/js tests, which haven't been migrated.
(we're only keeping the code here for later use)

@ericvergnaud ericvergnaud merged commit 5bed1e5 into antlr:dev Feb 7, 2024
10 checks passed
@ericvergnaud ericvergnaud deleted the fix-local-testing branch February 7, 2024 19:57
<module>tool</module>
<module>antlr5-maven-plugin</module>
<module>tool-testsuite</module>
<module>runtime-testsuite</module>
<module>runtime/Kotlin</module>
Copy link
Member

@KvanTTT KvanTTT Feb 7, 2024

Choose a reason for hiding this comment

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

I'm not sure that order of included modules affects compilations. Moreover, the current order is incorrect because runtime-testsuite module actually depends on runtime/Kotlin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maven's Reactor will take care of ordering modules based on dependencies, so the order doesn't matter.

@jimidle
Copy link
Contributor

jimidle commented Feb 8, 2024 via email

@ftomassetti
Copy link
Collaborator

Just as an update, I get the exact same results from two different machines (both macs)

@lppedd
Copy link
Collaborator

lppedd commented Feb 8, 2024

I'll give it ago this afternoon, let's see if I can repro too.

@lppedd
Copy link
Collaborator

lppedd commented Feb 8, 2024

I can run tests correctly with:

cd runtime-testsuite 
mvn -Dtest=kotlin.** test
mvn -Dtest=java.** test

Which is what CI does.

Running mvn test fails tho, it seems on JS tests, which we don't need anyway.

@ftomassetti
Copy link
Collaborator

Yes, that works (with the slight changes of adding " around the -Dtest parameter).

Are instead the tests of the maven plugin expected to fail?

@lppedd
Copy link
Collaborator

lppedd commented Feb 8, 2024

Are instead the tests of the maven plugin expected to fail?

Plugin's tests use takari-plugin-testing, which received its last meaningful update more than three years ago. At this point testing with anything above Maven 3.6 is likely to fail.

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.

5 participants