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.

Sign up for GitHub

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

Jump to bottom

Allow Test to be skipped when pom specifies maven.test.skip=true #1263

Merged
merged 2 commits into from
Feb 27, 2023
Merged

Allow Test to be skipped when pom specifies maven.test.skip=true #1263

merged 2 commits into from
Feb 27, 2023

Conversation

RoiSoleil
Copy link
Contributor

Fix : #1262

@laeubi
Copy link
Member

laeubi commented Feb 20, 2023

@RoiSoleil can you probably elaborate a bit more about the context (and probably also adding a testcase to verify this)?

@RoiSoleil
Copy link
Contributor Author

@RoiSoleil can you probably elaborate a bit more about the context (and probably also adding a testcase to verify this)?

When debugging a very big project, compiling tests can take a lot of time. Being able to disable this can save time to debug more rapidly. I will add a test for my code.

@RoiSoleil RoiSoleil force-pushed the master branch 2 times, most recently from bbfd34b to cc38286 Compare February 20, 2023 15:15
@RoiSoleil
Copy link
Contributor Author

@laeubi is that ok for you ?

laeubi
laeubi approved these changes Feb 20, 2023
View reviewed changes
Copy link
Member

@laeubi laeubi left a comment

Choose a reason for hiding this comment

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

This looks fine and all tests succeed, maybe @HannesWell or @mickaelistria like to review also.

@github-actions
Copy link

github-actions bot commented Feb 20, 2023 •
edited
Loading

Test Results

615 tests  +3   608 ✔️ +3   8m 23s ⏱️ +12s
  98 suites ±0       7 💤 ±0 
  98 files   ±0       0 ❌ ±0 

Results for commit 3004871. ± Comparison against base commit fadb4eb.

♻️ This comment has been updated with latest results.

@HannesWell
Copy link
Contributor

This looks fine and all tests succeed, maybe @HannesWell or @mickaelistria like to review also.

Yes, I would ike to review this, probably tomorrow.

@RoiSoleil
Copy link
Contributor Author

This looks fine and all tests succeed, maybe @HannesWell or @mickaelistria like to review also.

Yes, I would ike to review this, probably tomorrow.

Is it OK ?

HannesWell
HannesWell requested changes Feb 23, 2023
View reviewed changes
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Sorry for the delayed reply.

In general I think the change makes sense, but I think the way how it is implemented should be more general (which would make it simpler).
Please see the comments below.

@RoiSoleil RoiSoleil requested review from HannesWell and laeubi and removed request for mickaelistria, HannesWell and laeubi February 23, 2023 12:58
HannesWell
HannesWell reviewed Feb 25, 2023
View reviewed changes
Copy link
Contributor

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thanks for the update. This looks very good now, I especially appreciate the addition of even more test cases.

Just one remark regarding the conversion of boolean values below.
If that is applied this is IMO ready.

@HannesWell
Copy link
Contributor

Besides the code change, I think this should be mentioned in M2E's RELEASE_NOTES.
I think the change is reasonable (usually skipTests should be used to skip the test execution and they should still be compiled), but I think it could be unexpected for users and therefore it should be mentioned.
@RoiSoleil do you want to add such entry?

@RoiSoleil
Copy link
Contributor Author

@HannesWell it should be ok now ;)

@RoiSoleil RoiSoleil requested a review from HannesWell February 27, 2023 08:32
HannesWell
HannesWell approved these changes Feb 27, 2023
View reviewed changes
Copy link
Contributor

@HannesWell HannesWell 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 now. 👍🏽
Thank you for this contribution.

I have rephrased the Release-Notes entry a bit and will commit this with your change.

@RoiSoleil
Copy link
Contributor Author

Thank you, it's nice to see eclipse moving forward ;)

@HannesWell HannesWell merged commit f305fa5 into eclipse-m2e:master Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@laeubi laeubi laeubi approved these changes

@HannesWell HannesWell HannesWell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Allow Test to be skipped when pom specifies maven.test.skip=true
3 participants
@RoiSoleil @laeubi @HannesWell