From commits-return-9090-archive-asf-public=cust-asf.ponee.io@fineract.apache.org Sun May 17 05:04:16 2020 Return-Path: X-Original-To: archive-asf-public@cust-asf.ponee.io Delivered-To: archive-asf-public@cust-asf.ponee.io Received: from mail.apache.org (hermes.apache.org [207.244.88.153]) by mx-eu-01.ponee.io (Postfix) with SMTP id 1DF8E180626 for ; Sun, 17 May 2020 07:04:16 +0200 (CEST) Received: (qmail 49191 invoked by uid 500); 17 May 2020 05:04:15 -0000 Mailing-List: contact commits-help@fineract.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: dev@fineract.apache.org Delivered-To: mailing list commits@fineract.apache.org Received: (qmail 49182 invoked by uid 99); 17 May 2020 05:04:15 -0000 Received: from ec2-52-202-80-70.compute-1.amazonaws.com (HELO gitbox.apache.org) (52.202.80.70) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 17 May 2020 05:04:15 +0000 From: =?utf-8?q?GitBox?= To: commits@fineract.apache.org Subject: =?utf-8?q?=5BGitHub=5D_=5Bfineract=5D_ptuomola_commented_on_a_change_in_pull?= =?utf-8?q?_request_=23817=3A_Step_1_of_improving_SchedulerJobHelper_to_be_m?= =?utf-8?q?ore_reliable_and_prevent_flaky_tests_=28FINERACT-922=29?= Message-ID: <158969185518.19379.9352115295088339337.asfpy@gitbox.apache.org> Date: Sun, 17 May 2020 05:04:15 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit References: In-Reply-To: ptuomola commented on a change in pull request #817: URL: https://github.com/apache/fineract/pull/817#discussion_r426216912 ########## File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/common/SchedulerJobHelper.java ########## @@ -122,45 +139,112 @@ private static String runSchedulerJobAsJSON() { return runSchedulerJob; } - public void executeJob(String jobName) throws InterruptedException { - List allSchedulerJobsData = getAllSchedulerJobs(); - Assert.assertNotNull(allSchedulerJobsData); - + private int getSchedulerJobIdByName(String jobName) { + List> allSchedulerJobsData = getAllSchedulerJobs(); for (Integer jobIndex = 0; jobIndex < allSchedulerJobsData.size(); jobIndex++) { if (allSchedulerJobsData.get(jobIndex).get("displayName").equals(jobName)) { - Integer jobId = (Integer) allSchedulerJobsData.get(jobIndex).get("jobId"); + return (Integer) allSchedulerJobsData.get(jobIndex).get("jobId"); + } + } + throw new IllegalArgumentException("No such named Job (see org.apache.fineract.infrastructure.jobs.service.JobName enum):" + jobName); + } - // Executing Scheduler Job - runSchedulerJob(this.requestSpec, jobId.toString()); + @Deprecated // FINERACT-922 TODO Gradually replace use of this method with new executeAndAwaitJob() below, if it proves to be more stable than this one + public void executeJob(String jobName) throws InterruptedException { + // Stop the Scheduler while we manually trigger execution of job, to avoid side effects and simplify debugging when readings logs + updateSchedulerStatus(false); + + int jobId = getSchedulerJobIdByName(jobName); - // Retrieving Scheduler Job by ID - Map schedulerJob = getSchedulerJobById(jobId); - Assert.assertNotNull(schedulerJob); + // Executing Scheduler Job + runSchedulerJob(jobId); - // Waiting for Job to complete - while ((Boolean) schedulerJob.get("currentlyRunning") == true) { - Thread.sleep(15000); - schedulerJob = getSchedulerJobById(jobId); - Assert.assertNotNull(schedulerJob); - System.out.println("Job is Still Running"); - } + // Retrieving Scheduler Job by ID + Map schedulerJob = getSchedulerJobById(jobId); - List jobHistoryData = getSchedulerJobHistory(jobId); + // Waiting for Job to complete + while ((Boolean) schedulerJob.get("currentlyRunning") == true) { + Thread.sleep(15000); + schedulerJob = getSchedulerJobById(jobId); + assertNotNull(schedulerJob); + System.out.println("Job is Still Running"); + } - Assert.assertFalse("Job History is empty :( Was it too slow? Failures in background job?", jobHistoryData.isEmpty()); + List> jobHistoryData = getSchedulerJobHistory(jobId); - // print error associated with recent job failure (if any) - System.out.println("Job run error message (printed only if the job fails: " - + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorMessage")); - System.out.println("Job failure error log (printed only if the job fails: " - + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorLog")); + assertFalse("Job History is empty :( Was it too slow? Failures in background job?", jobHistoryData.isEmpty()); - // Verifying the Status of the Recently executed Scheduler Job - Assert.assertEquals("Verifying Last Scheduler Job Status", "success", - jobHistoryData.get(jobHistoryData.size() - 1).get("status")); + // print error associated with recent job failure (if any) + System.out.println("Job run error message (printed only if the job fails: " + + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorMessage")); + System.out.println("Job failure error log (printed only if the job fails: " + + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorLog")); + + // Verifying the Status of the Recently executed Scheduler Job + assertEquals("Verifying Last Scheduler Job Status", "success", + jobHistoryData.get(jobHistoryData.size() - 1).get("status")); + } - break; + /** + * Launches a Job and awaits its completion. + * @param jobName displayName (see {@link org.apache.fineract.infrastructure.jobs.service.JobName}) of Scheduler Job + * + * @author Michael Vorburger.ch + */ + public void executeAndAwaitJob(String jobName) { + Duration TIMEOUT = Duration.ofSeconds(30); + Duration PAUSE = Duration.ofMillis(500); + DateTimeFormatter df = DateTimeFormatter.ISO_INSTANT; // FINERACT-926 + Instant beforeExecuteTime = now().truncatedTo(ChronoUnit.SECONDS); + + // Stop the Scheduler while we manually trigger execution of job, to avoid side effects and simplify debugging when readings logs + updateSchedulerStatus(false); + + // Executing Scheduler Job + int jobId = getSchedulerJobIdByName(jobName); + runSchedulerJob(jobId); + + // Await JobDetailData.lastRunHistory [JobDetailHistoryData] jobRunStartTime >= beforeExecuteTime (or timeout) + await().atMost(TIMEOUT).pollInterval(PAUSE).until(jobLastRunHistorySupplier(jobId), lastRunHistory -> { + String jobRunStartText = lastRunHistory.get("jobRunStartTime"); + if (jobRunStartText == null) { + return false; } + Instant jobRunStartTime = df.parse(jobRunStartText, Instant::from); + return jobRunStartTime.equals(jobRunStartTime) || jobRunStartTime.isAfter(beforeExecuteTime); Review comment: Shouldn't the first condition be comparing beforeExecuteTime and jobRunStartTime? ########## File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/common/SchedulerJobHelper.java ########## @@ -122,45 +139,112 @@ private static String runSchedulerJobAsJSON() { return runSchedulerJob; } - public void executeJob(String jobName) throws InterruptedException { - List allSchedulerJobsData = getAllSchedulerJobs(); - Assert.assertNotNull(allSchedulerJobsData); - + private int getSchedulerJobIdByName(String jobName) { + List> allSchedulerJobsData = getAllSchedulerJobs(); for (Integer jobIndex = 0; jobIndex < allSchedulerJobsData.size(); jobIndex++) { if (allSchedulerJobsData.get(jobIndex).get("displayName").equals(jobName)) { - Integer jobId = (Integer) allSchedulerJobsData.get(jobIndex).get("jobId"); + return (Integer) allSchedulerJobsData.get(jobIndex).get("jobId"); + } + } + throw new IllegalArgumentException("No such named Job (see org.apache.fineract.infrastructure.jobs.service.JobName enum):" + jobName); + } - // Executing Scheduler Job - runSchedulerJob(this.requestSpec, jobId.toString()); + @Deprecated // FINERACT-922 TODO Gradually replace use of this method with new executeAndAwaitJob() below, if it proves to be more stable than this one Review comment: Should we not just replace the old implementation with the new, and skip with the gradual transition? Or are there concerns that the new one may not work in some cases? ########## File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/common/SchedulerJobHelper.java ########## @@ -51,21 +60,28 @@ public SchedulerJobHelper(final RequestSpecification requestSpec, final Response this.response202Spec = responseSpec; } - private List getAllSchedulerJobs() { + private List> getAllSchedulerJobs() { final String GET_ALL_SCHEDULER_JOBS_URL = "/fineract-provider/api/v1/jobs?" + Utils.TENANT_IDENTIFIER; System.out.println("------------------------ RETRIEVING ALL SCHEDULER JOBS -------------------------"); - final ArrayList response = Utils.performServerGet(requestSpec, response200Spec, GET_ALL_SCHEDULER_JOBS_URL, ""); + List> response = Utils.performServerGet(requestSpec, response200Spec, GET_ALL_SCHEDULER_JOBS_URL, ""); + assertNotNull(response); return response; } + private List getAllSchedulerJobDetails(Function, T> mapper) { + return getAllSchedulerJobs().stream().map(mapper).collect(Collectors.toList()); + } + public List getAllSchedulerJobIds() { - ToIntFunction mapper = map -> (Integer) map.get("jobId"); - return getAllSchedulerJobs().stream().mapToInt(mapper).boxed().collect(Collectors.toList()); + return getAllSchedulerJobDetails(map -> (Integer) map.get("jobId")); + } + + public List getAllSchedulerJobNames() { + return getAllSchedulerJobDetails(map -> (String) map.get("displayName")); } public Map getSchedulerJobById(int jobId) { final String GET_SCHEDULER_JOB_BY_ID_URL = "/fineract-provider/api/v1/jobs/" + jobId + "?" + Utils.TENANT_IDENTIFIER; - System.out.println("------------------------ RETRIEVING SCHEDULER JOB BY ID -------------------------"); Review comment: Just a cosmetic comment: with this, the logging from the different requests is not uniform. All the other calls log the "--- ---" text, this one logs the response. Is there a reason for this? ########## File path: fineract-provider/src/integrationTest/java/org/apache/fineract/integrationtests/common/SchedulerJobHelper.java ########## @@ -122,45 +139,112 @@ private static String runSchedulerJobAsJSON() { return runSchedulerJob; } - public void executeJob(String jobName) throws InterruptedException { - List allSchedulerJobsData = getAllSchedulerJobs(); - Assert.assertNotNull(allSchedulerJobsData); - + private int getSchedulerJobIdByName(String jobName) { + List> allSchedulerJobsData = getAllSchedulerJobs(); for (Integer jobIndex = 0; jobIndex < allSchedulerJobsData.size(); jobIndex++) { if (allSchedulerJobsData.get(jobIndex).get("displayName").equals(jobName)) { - Integer jobId = (Integer) allSchedulerJobsData.get(jobIndex).get("jobId"); + return (Integer) allSchedulerJobsData.get(jobIndex).get("jobId"); + } + } + throw new IllegalArgumentException("No such named Job (see org.apache.fineract.infrastructure.jobs.service.JobName enum):" + jobName); + } - // Executing Scheduler Job - runSchedulerJob(this.requestSpec, jobId.toString()); + @Deprecated // FINERACT-922 TODO Gradually replace use of this method with new executeAndAwaitJob() below, if it proves to be more stable than this one + public void executeJob(String jobName) throws InterruptedException { + // Stop the Scheduler while we manually trigger execution of job, to avoid side effects and simplify debugging when readings logs + updateSchedulerStatus(false); + + int jobId = getSchedulerJobIdByName(jobName); - // Retrieving Scheduler Job by ID - Map schedulerJob = getSchedulerJobById(jobId); - Assert.assertNotNull(schedulerJob); + // Executing Scheduler Job + runSchedulerJob(jobId); - // Waiting for Job to complete - while ((Boolean) schedulerJob.get("currentlyRunning") == true) { - Thread.sleep(15000); - schedulerJob = getSchedulerJobById(jobId); - Assert.assertNotNull(schedulerJob); - System.out.println("Job is Still Running"); - } + // Retrieving Scheduler Job by ID + Map schedulerJob = getSchedulerJobById(jobId); - List jobHistoryData = getSchedulerJobHistory(jobId); + // Waiting for Job to complete + while ((Boolean) schedulerJob.get("currentlyRunning") == true) { + Thread.sleep(15000); + schedulerJob = getSchedulerJobById(jobId); + assertNotNull(schedulerJob); + System.out.println("Job is Still Running"); + } - Assert.assertFalse("Job History is empty :( Was it too slow? Failures in background job?", jobHistoryData.isEmpty()); + List> jobHistoryData = getSchedulerJobHistory(jobId); - // print error associated with recent job failure (if any) - System.out.println("Job run error message (printed only if the job fails: " - + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorMessage")); - System.out.println("Job failure error log (printed only if the job fails: " - + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorLog")); + assertFalse("Job History is empty :( Was it too slow? Failures in background job?", jobHistoryData.isEmpty()); - // Verifying the Status of the Recently executed Scheduler Job - Assert.assertEquals("Verifying Last Scheduler Job Status", "success", - jobHistoryData.get(jobHistoryData.size() - 1).get("status")); + // print error associated with recent job failure (if any) + System.out.println("Job run error message (printed only if the job fails: " + + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorMessage")); + System.out.println("Job failure error log (printed only if the job fails: " + + jobHistoryData.get(jobHistoryData.size() - 1).get("jobRunErrorLog")); + + // Verifying the Status of the Recently executed Scheduler Job + assertEquals("Verifying Last Scheduler Job Status", "success", + jobHistoryData.get(jobHistoryData.size() - 1).get("status")); + } - break; + /** + * Launches a Job and awaits its completion. + * @param jobName displayName (see {@link org.apache.fineract.infrastructure.jobs.service.JobName}) of Scheduler Job + * + * @author Michael Vorburger.ch + */ + public void executeAndAwaitJob(String jobName) { + Duration TIMEOUT = Duration.ofSeconds(30); + Duration PAUSE = Duration.ofMillis(500); + DateTimeFormatter df = DateTimeFormatter.ISO_INSTANT; // FINERACT-926 + Instant beforeExecuteTime = now().truncatedTo(ChronoUnit.SECONDS); + + // Stop the Scheduler while we manually trigger execution of job, to avoid side effects and simplify debugging when readings logs + updateSchedulerStatus(false); + + // Executing Scheduler Job + int jobId = getSchedulerJobIdByName(jobName); + runSchedulerJob(jobId); + + // Await JobDetailData.lastRunHistory [JobDetailHistoryData] jobRunStartTime >= beforeExecuteTime (or timeout) + await().atMost(TIMEOUT).pollInterval(PAUSE).until(jobLastRunHistorySupplier(jobId), lastRunHistory -> { + String jobRunStartText = lastRunHistory.get("jobRunStartTime"); + if (jobRunStartText == null) { + return false; } + Instant jobRunStartTime = df.parse(jobRunStartText, Instant::from); + return jobRunStartTime.equals(jobRunStartTime) || jobRunStartTime.isAfter(beforeExecuteTime); + }); + + // Await JobDetailData.lastRunHistory [JobDetailHistoryData] jobRunEndTime to be both set and >= jobRunStartTime (or timeout) + Map finalLastRunHistory = await().atMost(TIMEOUT).pollInterval(PAUSE).until(jobLastRunHistorySupplier(jobId), lastRunHistory -> { + String jobRunEndText = lastRunHistory.get("jobRunEndTime"); + if (jobRunEndText == null) { + return false; + } + Instant jobRunEndTime = df.parse(jobRunEndText, Instant::from); + Instant jobRunStartTime = df.parse(lastRunHistory.get("jobRunStartTime"), Instant::from); + return jobRunEndTime.equals(jobRunStartTime) || jobRunEndTime.isAfter(jobRunStartTime); + }); + + // Verify triggerType + assertThat(finalLastRunHistory.get("triggerType"), is("application")); + + // Verify status & propagate jobRunErrorMessage and/or jobRunErrorLog (if any) + String status = finalLastRunHistory.get("status"); + if (!status.equals("success")) { + fail("Job status is not success: " + finalLastRunHistory.toString()); } + + // PS: Checking getSchedulerJobHistory() [/runhistory] is pointless, because the lastRunHistory JobDetailHistoryData is already part of JobDetailData anyway. + } + + @SuppressWarnings("unchecked") Review comment: Would be great to get rid of the SuppressWarnings... I wonder if there's any clever solution to achieve that? ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: users@infra.apache.org