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: multiple candidates for method isIgnored. #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

idan-at
Copy link

@idan-at idan-at commented Mar 24, 2025

Some runners, like Google's TestParameterInjector
(https://github.com/google/TestParameterInjector), cause a situation where isIgnored has multiple candidates, causing MethodUtils.invokeMethod to fail.

Some runners, like Google's TestParameterInjector
(https://github.com/google/TestParameterInjector), cause a situation
where isIgnored has multiple candidates, causing MethodUtils.invokeMethod
to fail.
@idan-at
Copy link
Author

idan-at commented Mar 24, 2025

@sbabcoc I'd really appreciate if you can submit and release this change. Apparently, this collides with TestParameterInjector, causing the test runner to fail.

@sbabcoc
Copy link
Owner

sbabcoc commented Mar 24, 2025

@idan-at Thank you for your submission! I really appreciate it. Please open an issue report that details the issue. It will be helpful to have a simple example that reproduces the issue which can be added to the unit test suite.

@idan-at
Copy link
Author

idan-at commented Mar 24, 2025

Done, described it here: #136

@idan-at
Copy link
Author

idan-at commented Mar 24, 2025

Regarding the test - I’m not sure where to add it. If you guide me I’ll add one to this PR

@sbabcoc
Copy link
Owner

sbabcoc commented Mar 24, 2025

@idan-at The test shown in your issue report is exactly what I need. I have a big meeting today, so I may not be able to dig into this until tomorrow.

@idan-at
Copy link
Author

idan-at commented Mar 24, 2025

Thank you @sbabcoc i highly appreciate it.

(I also see #121 is relevant for this)

@sbabcoc
Copy link
Owner

sbabcoc commented Mar 26, 2025

The root cause of the issues you're running into is that JUnit-Foundation doesn't work with TestParameterInjector yet. Swallowing the exception isn't going to solve this problem. I've fixed the issue with "isIgnored", only to run into the same failure with "describeChild" farther on.
I may not be able to use MethodUtils, which is convenient but apparently lacks the ability to work gracefully with overridden methods.

@idan-at
Copy link
Author

idan-at commented Mar 26, 2025

I see. This doesn’t seem straightforward from a quick online search. WDYT about stating that retries do not currently work with this runner, together with somehow allowing to disable it (either manually or automatically)

this will be helpful in my case, where I rely on this library only for global timeouts.

@sbabcoc
Copy link
Owner

sbabcoc commented Mar 26, 2025

I'm making progress, but a key feature (tracking of "atomic" tests) isn't working. I'll keep working on it tomorrow to see if I can get this working.

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.

2 participants