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

change default integration test goal action mapping to failsafe #4096

Merged
merged 1 commit into from
May 16, 2022

Conversation

ebarboni
Copy link
Contributor

This can be a fix for #4089 with the option to change de default action mapping for integration test.

@ebarboni ebarboni added this to the NB14 milestone May 10, 2022
@ebarboni ebarboni linked an issue May 10, 2022 that may be closed by this pull request
@matthiasblaesing
Copy link
Contributor

Does this do a full build? I expect an integration test to have the final deployment artifact available. My use-case are dropwizard applications, where I need to test the case when the final fat jar is build (basicly after package, when integration test is regularly run). I'll try to remember to check tomorrow whether or not integration tests are run in my setup. For me the drawback is, that unittests are generally fast, my integration tests run in the range of a 15 minutes, so I'm not sure whether I really want to run integrationtests by default.

@ebarboni
Copy link
Contributor Author

maybe we should go to pre-integration-test phase before calling failsafe:integration-test.
but the default action for unit test force plugin goal but not for IT test this is a bit inconsistent.

@neilcsmith-net
Copy link
Member

I'm not sure whether I really want to run integrationtests by default

@matthiasblaesing yes, please take a look back at #3470 as well as the report at #4089 This is about the ability to run / debug single test files or methods. This UI worked (kind of?!) in NB13 but not without additional configuration in NB14.

@neilcsmith-net neilcsmith-net added the Regression This used to work! label May 11, 2022
@ebarboni
Copy link
Contributor Author

@matthiasblaesing did you had time to test on your system to see if it's break your workflow ?

Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

I think it would be an error to merge this - I added a description to original issue and I'd bet, that the reported did not setup the maven project correctly (i.e. failsafe was not activated).

@neilcsmith-net
Copy link
Member

@matthiasblaesing surely the same argument could be made for the default unit test action? Which I assume actually got triggered for integration tests in NB13 as well? It seems to be a regression that this does nothing in NB14.

@matthiasblaesing
Copy link
Contributor

@neilcsmith-net not sure what you want to tell me.

Maven has a default binding of surefire:test to test. The same is not true for integration tests, but goals for integration tests and their verification do exist. As a developer you can tie into them or don't. Failsafe is very explicit that you have to configure it. If you don't do it, your project is broken. The change will at least break setups where people correctly setup their systems and don't use failsafe, but something different (the goal is changed).

@matthiasblaesing
Copy link
Contributor

So I tested in NB13 and it is not a regression. NB 13 executed integration tests as unittests, which for trivial cases looks as if it works, but immediately breaks down for complex cases. I added a longer version to the original issue, including a potential improvement.

@neilcsmith-net
Copy link
Member

@matthiasblaesing that NetBeans has a default binding explicitly to surefire:test in the run single test actions. While NB13 isn't correct in using that for integration tests, I think the goal should probably still be explicit. Are the default action properties correct to work with another IT plugin?

@matthiasblaesing
Copy link
Contributor

@neilcsmith-net I would argue, that the current "run single test" action is broken as is and needs to be updated to call into test. Could we please stop to pretend, that the IDE can lift the requirements, that developers have to understand their job and read the f*** documentation?

Running a single integration test works as long as you did a baseline setup of the environment. If you can't do that, your integration test is already broken and thus useless. Your suggestion breaks working test while only giving toy tests the benefit to pretend to work.

@matthiasblaesing
Copy link
Contributor

I also tested a build based on this PR and running an integration test from a clean checkout fails now, as core goals are skipped (in my case package). Integration tests are run after package (see https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#Lifecycle_Reference). With the current implementation the full maven cycle will be run and then the single integration test is run, with this PR most of the maven cycle is skipped. This is faster, but for me results in broken tests.

@neilcsmith-net
Copy link
Member

a clean checkout fails now, as core goals are skipped (in my case package). Integration tests are run after package

@matthiasblaesing yes, one of the questions Eric and I have both raised is what goal(s) needs to run first, presumably package for this to work?

I would argue, that the current "run single test" action is broken as is and needs to be updated to call into test ... Your suggestion breaks working test while only giving toy tests the benefit to pretend to work.

For the record, I agree with you here that if feasible we should just call test, and in fact map all actions to Maven goals rather than plugins. That would depend on improving default POM and looking at the properties passed in to make this work though.

Also for the record, this isn't my suggestion, but the action mappings should at least work in a consistent way (and this one could be made to function correctly).

As NB13 and NB14 are both broken here in different ways, it's not so much of a regression. IMO up to @ebarboni whether to update this PR with working pre-goals or not merge. Anything wider should be left for discussion in NB15.

@ebarboni
Copy link
Contributor Author

@matthiasblaesing @neilcsmith-net I changed the pre-goal of failsafe:integration-test to "pre-integration-test" as we have the
process-test-classes of surefire:test according to https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#Lifecycle_Reference ( I take the previous phase) .

I agree this only hotfix the issue. The menu on file propose test file and debug test file are enabled we should either edit the pom to add failsafe or disable the menuitem file are not "valid"

@neilcsmith-net
Copy link
Member

Thanks. Looks a good hotfix from my perspective! 👍 Let's discuss rest for NB15 (probably more than just adding to POM would be required though).

@matthiasblaesing matthiasblaesing self-requested a review May 16, 2022 15:31
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

I disagree, that this is an issue, but it will potentially break setups. As I don't have a real world example handy, I'll shut up now.

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.

Integration tests not started
3 participants