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

test: replace jre matching check with JUnit annotation #3704

Merged

Conversation

Artamm
Copy link
Contributor

@Artamm Artamm commented Nov 17, 2020

(Close #3682)
Some methods in test cases are replaced with annotations. In VariableTest class the annotation is also added on tests that use methods "since jdk_version".

@monperrus
Copy link
Collaborator

Cool!

@Artamm Artamm marked this pull request as ready for review November 18, 2020 23:38
@Artamm
Copy link
Contributor Author

Artamm commented Nov 18, 2020

I'm facing a problem in TestModule class. In this class are used @Test from junit-vintage, which ignore junit5 annotations (including @DisabledForJreRange). However, if I change import, the test case testSimpleModuleCanBeBuilt fails because instead of 2 modules it gets only 1 (simple_module_with_code is not included).
On the one hand, disabling jre is not important here and can be ignored, but maybe there is a way to fix this problem?

@monperrus
Copy link
Collaborator

thanks for the update.

the test case testSimpleModuleCanBeBuilt fails because instead of 2 modules it gets only 1 (simple_module_with_code is not included).

Seems we can simply change this assertion.

@MartinWitt
Copy link
Collaborator

MartinWitt commented Nov 19, 2020

Hi,
I`m surprised how well the annotation works :D
LGTM and CI seems green.

@Artamm
Copy link
Contributor Author

Artamm commented Nov 19, 2020

LGTM and CI seems green.

Hello. I think CI is green because now in TestModule is used junit4 @Test annotation, so in the test case there are 2 modules and checks pass (but @DisabledForJreRange is ignored). If replace with junit5 @test, the test case will have 1 module and check will fail, but the annotation will work.
If it's okay, I think that changing the assertion can work (like @monperrus said) . Or removing annotation, since in launcher is set jre version anyway.

@MartinWitt
Copy link
Collaborator

Or removing annotation, since in launcher is set jre version anyway.

If the test is green afterwards perfect solution, but changing the assertion is fine for me as well.

If replace with junit5 @test, the test case will have 1 module and check will fail, but the annotation will work.

This seems strange but debugging junit5 is not a nice task. I wouldn't prefer not to look into it.

@Artamm Artamm changed the title WIP: refactor (test) : replace jre matching with JUnit annotation refactor (test) : replace jre matching with JUnit annotation Nov 20, 2020
@monperrus
Copy link
Collaborator

Thanks a lot @Artamm LGTM

@MartinWitt OK to merge?

@MartinWitt
Copy link
Collaborator

Yes! Works perfect, nice work @Artamm !

@monperrus monperrus changed the title refactor (test) : replace jre matching with JUnit annotation test: replace jre matching check with JUnit annotation Nov 23, 2020
@monperrus monperrus merged commit c305bc7 into INRIA:master Nov 23, 2020
@monperrus
Copy link
Collaborator

Very nice contribution @Artamm !

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.

Refactor(Test): Remove jre matching from test cases and replace with junit annotation
3 participants