-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
YARN-11404. Add junit5 dependency to hadoop-mapreduce-client-app to fix few unit test failure #5295
Conversation
Change-Id: Ie8f21459b5334d5df65d5d0b2fbce385738c95b6
💔 -1 overall
This message was automatically generated. |
...mapreduce-client-app/src/test/java/org/apache/hadoop/mapred/TestTaskAttemptListenerImpl.java
Outdated
Show resolved
Hide resolved
...mapreduce-client-app/src/test/java/org/apache/hadoop/mapred/TestTaskAttemptListenerImpl.java
Show resolved
Hide resolved
Change-Id: I75e34743c13d62e4be1a52570425818a8e8b89bc
💔 -1 overall
This message was automatically generated. |
...ent-app/src/test/java/org/apache/hadoop/mapreduce/v2/app/job/impl/TestMapReduceChildJVM.java
Show resolved
Hide resolved
...educe-client-app/src/test/java/org/apache/hadoop/mapreduce/v2/app/rm/TestRMCommunicator.java
Show resolved
Hide resolved
...mapreduce-client-app/src/test/java/org/apache/hadoop/mapred/TestTaskAttemptListenerImpl.java
Show resolved
Hide resolved
Change-Id: Idd52f05750eaedaacc009ed8a5fc0dc645351714
💔 -1 overall
This message was automatically generated. |
Thanks @susheel-gupta for working on this. |
<dependency> | ||
<groupId>org.mockito</groupId> | ||
<artifactId>mockito-junit-jupiter</artifactId> | ||
<version>4.11.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have these versioned imports pulled up into the hadoop-project pom for (a) version maintenance and (b) ease of using an IDE to find where things are used. this is particularly important for mockito as it is so brittle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @steveloughran ,
Fair point.
@susheel-gupta Could you please open a follow-up jira for this?
Thanks.
couple of belated comments
I'm not going to suggest rolling this back -as it's in, and it was a big piece of work. It's just that in particular the code changes for jupiter assertions wasn't needed and it's potentially counter-productive. What I would propose is
|
Hi @steveloughran, |
thanks for the update, the pom pull would be welcome. and if there are other places we need to do that, don't be afraid to include in the same PR |
This seems to have broken the mapreduce tests. Reverting this fixed it locally for me, I think there ain't any followup ticket as Steve asked for either. Have reverted this. In case I have messed up, I will commit this again may be once I figure out the culprit, else this got sacrificed in my attempt to get the build green Tests passed locally
PS. The PR was closed not merged, not sure how & why but it was initially confusing to me |
problem here is that maven doesn't import transitive dependencies from a -test JAR, so if one module (mapreduce-client) adds a new mandatory dependency (here, junit jupiter stuff, maybe junit-mockito) then everything which imports the suite and uses it will break. We had that when we added assertJ tests to hadoop-common fs contract tests. Fix here is pom changes everywhere the test jar is referenced. |
Update:
From the Jira description : It looks like this PR was supposed to fix some tests but reverting this, didn't break them. |
Hi @ayushtkn , @susheel-gupta Anyway, the way forward in my opinion is to figure out if we can make the tests pass with your original commit + @steveloughran 's suggested pom fix. |
@ayushtkn |
Because those tests weren't executed. The way our Jenkins job is setup, it is like it runs the tests only for the modules which you touch as part of your change. Not even the modules which your change can impact, unfortunate but we can't run the entire test suites for all PRs, it would take around 20 hrs or more for it :) |
Description of PR
How was this patch tested?
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?