-
Notifications
You must be signed in to change notification settings - Fork 419
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
TEZ-4472: Use unique DAG names in tests. #293
Conversation
Change-Id: Ibbec29dc3ac57414943e9cb35121187c2251cf71
This comment was marked as outdated.
This comment was marked as outdated.
Change-Id: Ie57c9b944d08f107b7b2f8e839aed6392adacb27
Fixed test, the checkstyle warnings in |
This comment was marked as outdated.
This comment was marked as outdated.
LOG.info("Setting up mixed edge dag plan"); | ||
org.apache.tez.dag.api.DAG dag = org.apache.tez.dag.api.DAG.create("MixedEdges"); | ||
org.apache.tez.dag.api.DAG dag = | ||
org.apache.tez.dag.api.DAG.create("Dag-MixedEdges-" + dagNeme); |
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.
typo: neme
@ayushtkn: this patch covers much more test classes than I expected, which is a huge work thanks! |
@@ -203,12 +203,12 @@ TezClientForTest configureAndCreateTezClient(Map<String, LocalResource> lrs, boo | |||
|
|||
@Test (timeout = 5000) | |||
public void testTezclientApp() throws Exception { | |||
testTezClient(false, true); | |||
testTezClient(false, true, "testTezclientApp"); |
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.
I know this patch is not about that, but...: can you please fix all occurrences of "Tezclient" -> "TezClient"?
feel free to create a new ticket if you want
Change-Id: Ib1129ed8b9dc2edfea57c365551d466b3e6eade8
thanks @ayushtkn ! one more class to address here, can you please check TestLocalMode too? I remember that was root cause of my headache originally that I workarounded with something like (with respecting the junit parameters), can you adapt it?
and:
|
Change-Id: I611eb74577643feebf7dbf0aa37d6d41890cd5d6
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.
+1
pending tests
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
That checkstyle isn’t on me, I just touched that line, test failure doesn’t look related either |
agreed, TestTaskScheduler failure is crazy, nothing to do with this patch, we don't need a new precommit |
No description provided.