-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
[Feature-11473][Task]Support test task #11670
Conversation
…Test-Task # Conflicts: # dolphinscheduler-worker/src/main/java/org/apache/dolphinscheduler/server/worker/runner/TaskExecuteThread.java # dolphinscheduler-worker/src/test/java/org/apache/dolphinscheduler/server/worker/runner/TaskExecuteThreadTest.java
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
Kudos, SonarCloud Quality Gate passed! |
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
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.
@wen-hemin Hi, I think this PR has some problems to solve. It should not be merged so quickly. cc @caishunfeng @ruanwenjun @zhongjiajie
public static final int TEST_FLAG_NO = 0; | ||
public static final int TEST_FLAG_YES = 1; |
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.
It's better to create enumeration for it instead of this.
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.
Sure, some constants in this class can also be changed to enumeration.
@@ -17,6 +17,7 @@ | |||
|
|||
package org.apache.dolphinscheduler.dao.entity; | |||
|
|||
import com.baomidou.mybatisplus.annotation.*; |
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.
This should be avoided. cc @EricGao888
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.
@SbloodyS That's pretty wired. We have a step in Spotless to check and block wildcard imports. Usually CI would detect it and block this PR. The reason why it didn't block it may be our current setting for ratchet
.
Lines 660 to 664 in e1b55db
<replaceRegex> | |
<name>Remove wildcard imports</name> | |
<searchRegex>import\s+[^\*\s]+\*;(\r\n|\r|\n)</searchRegex> | |
<replacement>$1</replacement> | |
</replaceRegex> |
For details, see: #11412
Map<String, Object> taskDefinitionParams = JSONUtils.parseObject(taskInstance.getTaskDefine().getTaskParams(), new TypeReference<Map<String, Object>>() { | ||
}); | ||
Map<String, Object> taskInstanceParams = JSONUtils.parseObject(taskInstance.getTaskParams(), new TypeReference<Map<String, Object>>() { | ||
}); |
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.
We should avoid using map here.
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.
Map can access data more easily.
* [improve] support test tasks * [improve] support test tasks * [improve] support test tasks * [improve] support test tasks * [improve] support test tasks * Update TaskExecuteThread.java * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * Update DataSource.java * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * Update messages_zh_CN.properties * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * Update messages.properties * Update messages_en_US.properties * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks
}) | ||
@GetMapping(value = "/list") | ||
@ResponseStatus(HttpStatus.OK) | ||
@ApiException(QUERY_DATASOURCE_ERROR) | ||
@AccessLogAnnotation(ignoreRequestArgs = "loginUser") | ||
public Result queryDataSourceList(@ApiIgnore @RequestAttribute(value = Constants.SESSION_USER) User loginUser, | ||
@RequestParam("type") DbType type) { | ||
Map<String, Object> result = dataSourceService.queryDataSourceList(loginUser, type.ordinal()); | ||
@RequestParam("type") DbType type, |
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.
Another interesting thing here is this PR add a new parameter testFlag
to /datasources/list
but didn't update the corresponding UT and the CI somehow passed. After I upgraded jUnit locally, this UT case failed. cc @SbloodyS @kezhenxu94
Lines 127 to 143 in e1b55db
@ParameterizedTest | |
@CsvSource({ | |
"type, MYSQL" | |
}) | |
public void testQueryDataSourceList(String key, String dbType) throws Exception { | |
MultiValueMap<String, String> paramsMap = new LinkedMultiValueMap<>(); | |
paramsMap.add(key,dbType); | |
MvcResult mvcResult = mockMvc.perform(get("/datasources/list") | |
.header("sessionId", sessionId) | |
.params(paramsMap)) | |
.andExpect(status().isOk()) | |
.andExpect(content().contentType(MediaType.APPLICATION_JSON)) | |
.andReturn(); | |
Result result = JSONUtils.parseObject(mvcResult.getResponse().getContentAsString(), Result.class); | |
Assert.assertEquals(Status.SUCCESS.getCode(),result.getCode().intValue()); | |
logger.info(mvcResult.getResponse().getContentAsString()); | |
} |
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.
@insist777 please take a look. |
* [improve] support test tasks * [improve] support test tasks * [improve] support test tasks * [improve] support test tasks * [improve] support test tasks * Update TaskExecuteThread.java * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * try solve e2e q * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * Update DataSource.java * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * Update messages_zh_CN.properties * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * Update messages.properties * Update messages_en_US.properties * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks * [Feature] support test tasks
Hi, do we have a plan about which version will public this pr? |
3.2.0 |
Thx |
We should keep our step as less as possible, for now we have to test datasource and binding it to prod datasource, I think is will make our users do not thing to use datasource, so I do like to revert it and keep step as less as possible related to apache#11670
We should keep our step as less as possible, for now, we have to test datasource and binding it to the prod datasource, I think it will make our users do not thing to use datasource, so I do like to revert it and keep step as less as possible related to #11670
We should keep our step as less as possible, for now, we have to test datasource and binding it to the prod datasource, I think it will make our users do not thing to use datasource, so I do like to revert it and keep step as less as possible related to apache#11670
We should keep our step as less as possible, for now, we have to test datasource and binding it to the prod datasource, I think it will make our users do not thing to use datasource, so I do like to revert it and keep step as less as possible related to apache#11670
Purpose of the pull request
close #11473
Brief change log
support test tasks: The front-end shows. The back-end API module mainly modifies some interfaces. The master module mainly replaces the test data source for inspection. The worker module mainly verifies whether the test data source is replaced successfully
Verify this pull request
This pull request is code cleanup without any test coverage.