Remove daemon, improve tests, add tests, fix bugs #137
Conversation
In LSFJobManager: - Add the LSF_COMMAND_QUERY_EXTENDED_STATES constant, which will be used for the extended query. For short status queries, the former variable will be used. This vastly reduces the size of the json output. - Remove the DateTimeFormatter field and replaced it with a DateTimeHelper instance. - Added a custom parseTime() method, which will take care of LSF specific DateTime parsing issues (the year is always missing...) - Renamed queryJobInfo() to convertJobDetailsMapToGenericJobInfoobject() to match the methods function. - in runBJobs(), select the queryCommand based on the query type (extended or not). - in runBJobs(), add a filtering step to filter out old job entries. In Roddy the current default is 10min. - Move the error reporting code from convertBJobsJsonOutputToResultMap() to runBJobs() - Add the filterJobMapByAge() method Reworked the LSFJobManagerSpec class: - Moved JSON content to resource files - Merged and simplified test code - Added tests Merge branch 'fix-bjobs-parser-test' into AddDurationBasedBJobsFilter
src/main/groovy/de/dkfz/roddy/execution/jobs/JobManagerOptions.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy
Outdated
Show resolved
Hide resolved
src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy
Outdated
Show resolved
Hide resolved
src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManagerSpec.groovy
Outdated
Show resolved
Hide resolved
src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/queryExtendedJobStateByIdTest.json
Show resolved
Hide resolved
src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManagerSpec.groovy
Outdated
Show resolved
Hide resolved
src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManagerSpec.groovy
Show resolved
Hide resolved
Update gradle wrapper to 5.1.1
Updated the dependency for RoddyToolLib to 2.1.0
In GenericJobInfo, change field tool of type File to command of type
String. This better reflects that e.g. LSF can also execute commands. A
tool can always be seen as a command whereas a command is not
necessarily a tool.
In JobManagerOptions, rename maxAgeOfJobsForJobQueries to
maxTrackingTimeForFinishedJobs, as the new name is much more expressive.
In BatchEuphoriaJobManager and its subclasses, rename the aforementioned
field as well.
In PBSCommandParser and LSFCommandParser, do a bit of code cleanup and
adapt code.
In LSFJobManager:
- Add the stripAwayStatusInfo method for time string. This gets rid of
the trailing status code in some timing info reported by LSF.
- Change the return value of runBjobs to return a nested map of type
Map<String,String>. This makes some followup code much cleaner.
- Changed convertJobDetailsMapToGenericJobInfoobject a lot:
- Fixed a typo.
- Got rid of all the "as String" casts.
- Reordered the entries and grouped them logically.
- Extracted code duplicates to custom methods.
In LSFJobManagerSpec:
- Renamed "queryJobInfo, bjobs JSON output with lists" to
"Test convertJobDetailsMapToGenericJobInfoObject"
- Added the "test queryExtendedJobStateById with overdue date" method
which tests for a NullPointerException.
src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy
Show resolved
Hide resolved
| @@ -102,7 +114,7 @@ class LSFJobManager extends AbstractLSFJobManager { | |||
|
|
|||
| } | |||
|
|
|||
| Map<BEJobID, Map<String, Object>> runBjobs(List<BEJobID> jobIDs, boolean extended) { | |||
| Map<BEJobID, Map<String, String>> runBjobs(List<BEJobID> jobIDs, boolean extended) { | |||
There was a problem hiding this comment.
Would you add a description of the output map structure?
| @@ -121,12 +133,12 @@ class LSFJobManager extends AbstractLSFJobManager { | |||
| throw new BEException(error) | |||
| } | |||
|
|
|||
| Map<BEJobID, Map<String, Object>> result = convertBJobsJsonOutputToResultMap(resultLines.join("\n")) | |||
| return filterJobMapByAge(result, maxAgeOfJobsForQueries) | |||
| Map<BEJobID, Map<String, String>> result = convertBJobsJsonOutputToResultMap(resultLines.join("\n")) | |||
There was a problem hiding this comment.
description of output map structure
| } | ||
|
|
||
| static Map<BEJobID, Map<String, Object>> convertBJobsJsonOutputToResultMap(String rawJson) { | ||
| Map<BEJobID, Map<String, Object>> result = [:] | ||
| static Map<BEJobID, Map<String, String>> convertBJobsJsonOutputToResultMap(String rawJson) { |
| ZonedDateTime safelyParseTime(String time) { | ||
| if (time) | ||
| return withCaughtAndLoggedException { | ||
| return parseTime(time) |
There was a problem hiding this comment.
is the inner return necessary?
| @@ -60,11 +58,11 @@ class PBSCommandParser { | |||
|
|
|||
| if (!commandString.startsWith("qsub")) return // It is obviously not a PBS call | |||
|
|
|||
| String[] splitted = line.splitBy(" ").findAll { it } | |||
| Collection<String> splitted = line.splitBy(" ").findAll { it } | |||
There was a problem hiding this comment.
Exactly this is the reason I would prefer a dedicated Command class. Parsing the command is much more complex and failure prone and as implemented here makes a lot of assumptions about the command that would be completely unneccary if we'd use a simple dedicated command value-type.
| script = splitted[-1] | ||
| jobName = "not readable" | ||
|
|
||
| for (int i = 0; i < splitted.length - 1; i++) { | ||
| for (int i = 0; i < splitted.size() - 1; i++) { | ||
| String option = splitted[i] |
There was a problem hiding this comment.
This will fail on e.g.
"--commandArgs '-opt1 -opt2 -- rest inner args' --onotherOuterOption"
Update the dependency to RoddyToolLib to 2.1.1 // Library classes Renamed GenericJobInfo to ExtendedJobInfo Make both JobInfo classes use the @EqualsAndHashCode annotation to improve testability of code with (Extended)JobInfo objects. In ExtendedJobInfo: - Reordered the variable in attempt to group them better. - Renamed some of the variables to better match their function. - Make the class extend JobInfo and move jobID, endTime and jobState to the new class. Removed tracking and daemon fields from all classes. The daemon is now a conceptual idea which can be implemented by software using BE. In BatchEuphoriaJobManager: - Unify get(Extended)JobInfo method layout. - Implemented most methods in the class and made them final, add tests for them. In ClusterBasedJobManager: - Made withCaughtAndLoggedException public. - Added the toJobID(), safelyParseColonSeparatedDuration(), safelyParseTime() and the abstract parseTime() methods. In GridEngineBasedJobManager: - Add parseTime() method - Rename queryJobStates() to queryJobInfo() - Extract the assembleJobInfoQueryCommand() method from queryJobInfo() The method will assemble "qstat [-u] [id] [id] [...]" by default and is overridden in SGEJobManager. - Removed processQstatOutputFromPlainText(), we only parse from xml, so this was redundant and actually never used. - Add safe cast and convert methods for BufferValue, Integer, Duration TimeUnit and others. They will safely convert/case a value from a String. - Moved the processQstatOutputFromXML() method to PBSJobManager, SGE has a completely different implementation! In SGEJobManager: - Override assembleJobInfoQueryCommand() - Add convertJobInfoMapToExtendedMap() - Override queryExtendedJobInfo(), this works in a multi step process, first it will call query(All)JobInfo() to get basic information about jobs. Why? Because SGE provides you with different output depending whether active jobs can be found or not. Detailed explanations are in the method. After the first step, extended information are queried and processed. - Add parseGPathJobInfoElement(), which will parse a job/element entry in extended qstat xml output - Add extractAndParseTime() and extractStatisticsValue() In PBSJobManager: - Override queryExtendedJobInfo() - Add processQstatOutputFromXMLNodeChild(), which will parse a job entry in the extended qstat xml output. Actually it was moved here from GridEngineBasedJobManager. Also the contents are reordered to match the corresponding methods in LSFJobManager and SGEJobManager In LSFJobManager: - Reduce the returned JSON result in queryJobInfo() to the most essential fields: jobid, job_name, stat and finish_time. - Improve the parseTime method to cover several versions of possible LSF datetime reports. Detailed explanations are in the method comments - Improve stripAwayStatusInfo() - Remove queryExtendedJobStateById(), its in BEJobManager - Override queryJobInfo() and queryExtendedJobInfo(), remove queryJobStates - Remove filterJobMapByAge(). This is a method for a daemon and not covered by BatchEuphoria anymore. - Rework convertJobDetailsMapToGenericJobInfoObject() further - Remove safelyParseColonSeparatedDuration() and safelyParseTime(), they are in a super class. In LSFRestJobManager: - Override queryExtendedJobInfo() - Just saw, that a lot of work is still necessary in this class. But please review the rest first. // Test classes and files Add a lot of resource files, they are mostly extracted from tests and this is done to make the test layout more structured and the tests more readable. To load a resource file, utilize the new BETestBaseSpec class, which has the getResourceFile() method and a test for it. Created the JobManagerImplementationBaseSpec class, which is now a base specification for all job manager specific Spock specs: - The method createJobManagerWithModifiedExecutionService() can be used to create a job manager, which will use the passed resource for queryJobInfo() or queryExtendedJobInfo() test cases. It will create a job manager of type T! T needs to be set, when the Spec is extended - The method createJobManager() will create a very basic job manager of type T - Also there is a list of possible methods / test features, which are meant to bring some basic identical structure to all job manager tests - The class is tested in JobManagerImplementationBaseSpecSpec Create the BatchEuphoriaJobManagerSpec: - Make it implement JobManagerImplementationBaseSpec - Add tests for the different final query methods in BatchEuphoriaJobManager In GridEngineBasedJobManagerSpec, extend test for getExecutionHosts() In ClusterJobManagerSpec: - Add tests for parseColonSeparatedHHMMSSDuration() - Fix prepareClassLogger() - Rename tests Add the SGEJobManagerSpec: - Has stubs for most of the tests in JobManagerImplementationBaseSpec - Has several tests for query(Extended)JobInfo() which try to tackle the slightly special behaviour of queryExtendedJobInfo() In PBSJobManagerSpec: - Remove all the XML variables and move content to resource files. - Add stubs for tests in JobManagerImplementationBaseSpec - Make tests use the new equals method of (Extended)JobInfo. - Improved layout and readability of some tests. - Move the parseColonSeparatedHHMMSSDuration() tests to ClusterJobManagerSpec In PBSCommandParserTest: - Reduce code by utilizing the TestExecutionService for the PBSJobManager. In LSFJobManagerSpec: - Remove the getResourceFile() method. Its in a super class. - Make the class extend JobManagerImplementationBaseSpec and create test stubs - Spockify the parseTime() test and cover all cases and also malformatted strings - Make the query(E)JI() tests use the new equals feature of JobInfo - Remove the filterJobMapByAge test Create a test class LSFRestJobManagerSpec. It is not implemented yet!
dankwart-de
changed the title
There was a problem hiding this comment.
Generally, remove methods named doSomethingByTypeX(TypeX x) as this introduces unnecessarily long method names. The compiler can select the right method and the IDE shows the types (or the compiler breaks, if the type is not found. The method name should only describe the semantics, not the argument types. For the return types the situation is different, as the compiler does not dispatch on return types -- here taking the return type into the method name is unavoidable (in Groovy).
I admit I have not compared the old and new code and checked the plausibility of the new code (it's quite a lot!). I also only skimmed over the tests.
Please also have a look at the comments on the left side, because sometimes my comments are found there.
There are a lot of public methods. Was the idea not to user more package scope?
In general, I am very happy with the changes. Lots of small clear methods, documentation, tests. Just like it should be 8-D
| @@ -146,8 +119,13 @@ abstract class BatchEuphoriaJobManager<C extends Command> { | |||
| } | |||
| } | |||
|
|
|||
| static final assertListIsValid(List list) { | |||
There was a problem hiding this comment.
The name if this method is much to generic (What does "valid" mean?). Rename to e.g. assertNonNullElementList or so. Furthermore, the place for this list is wrong. It has no relation to the job manager and should be e.g. in a helper/service class.
One could then use static imports and achieve the same syntax.
An even better way would be to have a list collection that checks for non-null elements already when elements are entered, which would raise an error already closer to the source of the error.
| assertListIsValid(jobs) | ||
|
|
||
| Map<BEJobID, BEJob> jobsByID = jobs.collectEntries { | ||
| def job = it |
There was a problem hiding this comment.
Why not
jobs.collectEntries { job -> [job.jobID, job] }| // Collect the result and fill in empty entries with UNKNOWN | ||
| return jobIDs.collectEntries { | ||
| BEJobID jobID -> | ||
| [jobID, result[jobID] ?: new JobInfo(jobID)] |
There was a problem hiding this comment.
The default state with new is UNKNOWN, right? This is not clear from the code. Maybe set the state explicitly here, because here it is imporant to see what is happening. The code is reasonably easy to allow for removal of the comment, then!
| } | ||
|
|
||
| /** | ||
| * Queries the status of all jobs. | ||
| * Needs to be overriden by JobManager implementations. | ||
| * jobIDs may be null or [] which means, that the query is not for specific jobs but for all available (except for |
There was a problem hiding this comment.
I actually have problems with both special arguments null and []. Both can occur in error situations and I question that you want the method to really silently return all jobs. null is certainly not a special value, but an error. But even worse [] may even be a valid request, but instead of silently returning an empty list as result, the method should return everything!
For an API method I would strongly advise against this (while it may be fine for an private method).
I would rather like to see two methods, one for querying specific jobs from a list, and one for querying all jobs.
| Map<BEJobID, JobState> result = queryJobStatesUsingCache(jobIds, forceUpdate) | ||
| return jobIds.collectEntries { BEJobID jobId -> [jobId, result[jobId] ?: JobState.UNKNOWN] } | ||
| final Map<BEJobID, JobInfo> queryAllJobInfo() { | ||
| queryJobInfo(null) |
There was a problem hiding this comment.
Consider using [] here. See comment below.
| // jobInfo.queue // Info is not available | ||
|
|
||
| // Getting cores and nodes is a bit tricks. Nodes are not in the xml, cores via JB_pe_range | ||
| String requestedCoresRaw = job["JB_pe_range"]["ranges"]["RN_min"]?.toString() |
There was a problem hiding this comment.
Can we omit the requested prefixes?
| "999" | UNKNOWN | ||
| "1000" | RUNNING | ||
| "1001" | SUSPENDED | ||
| "1002" | COMPLETED_SUCCESSFUL |
There was a problem hiding this comment.
Should we actually also start fixing the states? Maybe soon. The states currently code two pieces of information in the name, namely status according to batch processing system (RUNNING, UNKOWN, COMPLETED) and exit code (SUCCESSFUL, FAILURE). We should absorb the exit code into a field.
| expected | mem | cores | nodes | walltime | ||
| "-l mem=1024M -l walltime=00:01:00:00 -l nodes=1:ppn=2" | 1024 | 2 | 1 | "h" | ||
| "-l walltime=00:01:00:00 -l nodes=1:ppn=1" | null | null | 1 | "h" | ||
| "-l mem=1024M -l nodes=1:ppn=2" | 1024 | 2 | null | null |
There was a problem hiding this comment.
nodes == null? nul means use the default, right?
For clarity I'd prefer to see the inputs on the left and the expect on the right (but that is nitpicking :-P)
|
|
||
| def jobManager = createJobManagerWithModifiedExecutionService("processExtendedQstatXMLOutputWithQueuedJob.xml") | ||
|
|
||
| ExtendedJobInfo expected = getGenericJobInfoObjectForTests() |
There was a problem hiding this comment.
getExtendedJobInfoobjectForTests()
| </job_sublist> | ||
| <job_sublist> | ||
| <VA_variable>CONFIG_FILE</VA_variable> | ||
| <VA_value>/home/heinold/temp/roddyLocalTest/testproject/rpp/stds/roddyExecutionStore/exec_190204_103439242_heinold_testCorrect2/RoddyTest_testScript_1.parameters</VA_value> |
There was a problem hiding this comment.
Is it o.k. to have your names in the code?
|
@coderabbitai review |
WalkthroughThe changes primarily revolve around the refactoring and enhancement of the job management system in a Gradle-based project. The job management system has been updated to use Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 29
Configuration used: CodeRabbit UI
Files ignored due to filter (15)
- gradle/wrapper/gradle-wrapper.jar
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/convertBJobsResultLinesToResultMapTest.json
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/queryExtendedJobStateByIdEmptyTest.json
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/queryExtendedJobStateByIdTest.json
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/queryExtendedJobStateByIdWithoutListsTest.json
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/queryJobStateByIdTest.json
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/pbs/processExtendedQstatXMLOutputTests.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/pbs/processExtendedQstatXMLOutputWithFinishedJob.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/pbs/processExtendedQstatXMLOutputWithQueuedJob.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/pbs/queryExtendedJobInfoWithEmptyXML.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/pbs/queryExtendedJobInfoWithPlaceholderReplacement.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/queryExtendedJobStatesWithFinishedJob.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/queryExtendedJobStatesWithJobWithPipedScript.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/queryExtendedJobStatesWithQueuedJobs.xml
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/queryExtendedJobStatesWithUnknownJobs.xml
Files selected for processing (45)
- build.gradle (2 hunks)
- gradle/wrapper/gradle-wrapper.properties (1 hunks)
- gradlew (1 hunks)
- gradlew.bat (1 hunks)
- src/main/groovy/de/dkfz/roddy/config/ResourceSet.groovy (1 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/BEFakeJobID.groovy (1 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy (1 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (15 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/ExtendedJobInfo.groovy (1 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/JobInfo.groovy (1 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/JobManagerOptions.groovy (2 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/QueryJobStatesFilter.groovy (1 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy (1 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (4 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (4 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFCommandParser.groovy (3 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (6 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManager.groovy (10 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParser.groovy (3 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (4 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (3 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/slurm/SlurmJobManager.groovy (3 hunks)
- src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (2 hunks)
- src/test/groovy/de/dkfz/roddy/batcheuphoria/BETestBaseSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/batcheuphoria/jobs/JobManagerOptionsTest.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (3 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManagerSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/ExtendedJobInfoTest.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (4 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManagerSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManagerSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/JobManagerImplementationBaseSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/JobManagerImplementationBaseSpecSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManagerSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManagerSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParserTest.groovy (3 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManagerSpec.groovy (1 hunks)
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManagerSpec.groovy (1 hunks)
- src/test/resources/de/dkfz/roddy/batcheuphoria/getResourceFileTest.txt (1 hunks)
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/bjobsJobTemplatePart1.txt (1 hunks)
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/bjobsJobTemplatePart2.txt (1 hunks)
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/createJobManagerWithModifiedExecutionServiceTest.txt (1 hunks)
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/pbs/createJobManagerWithModifiedExecutionServiceTest.txt (1 hunks)
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/createJobManagerWithModifiedExecutionServiceTest.txt (1 hunks)
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/simpleQStatOutput.txt (1 hunks)
Files skipped from review due to trivial changes (17)
- gradle/wrapper/gradle-wrapper.properties
- gradlew
- gradlew.bat
- src/main/groovy/de/dkfz/roddy/config/ResourceSet.groovy
- src/main/groovy/de/dkfz/roddy/execution/jobs/BEJobID.groovy
- src/main/groovy/de/dkfz/roddy/execution/jobs/ExtendedJobInfo.groovy
- src/main/groovy/de/dkfz/roddy/execution/jobs/QueryJobStatesFilter.groovy
- src/main/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommand.groovy
- src/test/groovy/de/dkfz/roddy/batcheuphoria/jobs/JobManagerOptionsTest.groovy
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManagerSpec.groovy
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/JobManagerImplementationBaseSpec.groovy
- src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManagerSpec.groovy
- src/test/resources/de/dkfz/roddy/batcheuphoria/getResourceFileTest.txt
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/bjobsJobTemplatePart1.txt
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/createJobManagerWithModifiedExecutionServiceTest.txt
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/pbs/createJobManagerWithModifiedExecutionServiceTest.txt
- src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/createJobManagerWithModifiedExecutionServiceTest.txt
Additional comments: 83
build.gradle (2)
46-54: The Jackson libraries have been added as dependencies. Ensure that these libraries are used in the codebase and that the version 2.9.7 is compatible with the rest of the project.
63-67: The dependency on
RoddyToolLibhas been updated to version 2.1.1. Make sure that this version is compatible with the rest of the project and that all necessary changes have been made to accommodate any differences between this version and the previous one.src/main/groovy/de/dkfz/roddy/execution/jobs/BEFakeJobID.groovy (1)
- 13-19: The class
BEFakeJobIDnow extendsBEJobIDinstead ofBEJobID.FakeJobID. Ensure that this change doesn't break any existing functionality that relied onBEFakeJobIDinheriting fromBEJobID.FakeJobID. Also, verify that all instances ofBEFakeJobIDare updated to reflect this change.The rest of the code seems fine, assuming that the methods and properties are being used correctly in the context of the larger application.
src/main/groovy/de/dkfz/roddy/execution/jobs/JobManagerOptions.groovy (2)
17-34: The removal of
createDaemonis consistent with the PR summary. Ensure that all references to this variable have been removed from the codebase.87-98: The default values for
requestStorageIsEnabledandpassEnvironmenthave been changed. Ensure that this does not affect any existing functionality that relies on these variables.src/test/groovy/de/dkfz/roddy/execution/jobs/BEIntegrationTest.groovy (2)
70-75: The
createDaemonmethod has been removed fromJobManagerOptions. Ensure that this change does not affect the behavior of thecreateJobManagerFormethod.146-159: The
queryJobStatusmethod has been replaced withqueryJobInfoByJob. Ensure that the returnedJobInfoobjects are correctly used in theensureProperJobStatesmethod.- def status = jobManager.queryJobStatus(jobList, true) + def jobInfo = jobManager.queryJobInfoByJob(jobList)src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/sge/SGEJobManager.groovy (3)
7-24: No changes needed. The import statements and class declaration are correct.
37-52: The method names have been updated to reflect their new functionality. Ensure that all calls to these methods throughout the codebase have been updated to match the new names. The
parseGenericJobInfomethod currently returns null, which might cause NullPointerExceptions if the return value is used without null checks. Consider throwing aNotImplementedExceptionor similar to indicate that this method needs to be implemented.- ExtendedJobInfo parseGenericJobInfo(String command) { - return null + throw new UnsupportedOperationException("Method not implemented yet.") }
- 131-202: The new methods
assembleJobInfoQueryCommand,convertJobInfoMapToExtendedMap,queryExtendedJobInfo,parseGPathJobInfoElement,extractAndParseTime, andextractStatisticsValuehave been added. These methods seem to be correctly implemented according to their comments. However, it's important to ensure that they are thoroughly tested, especiallyqueryExtendedJobInfoandparseGPathJobInfoElementwhich contain complex logic.Overall, the changes in this file seem to be well-implemented and follow good coding practices. However, it's important to ensure that all methods are thoroughly tested, especially those with complex logic.
src/main/groovy/de/dkfz/roddy/execution/jobs/BatchEuphoriaJobManager.groovy (5)
27-30: The
executionServicevariable has been made final, which means it can only be assigned once. This is a good practice for variables that are not supposed to change after initialization.101-103: The
addToListOfStartedJobs()method has been deprecated. Ensure that this method is not being used elsewhere in the codebase, or that those uses have been updated accordingly.144-152: The
queryJobStatus()method has been replaced withqueryJobInfoByJob()andqueryJobInfoByID(). These new methods returnJobInfoobjects instead ofJobStateobjects. This change should provide more detailed information about jobs.164-196: The
queryJobStatusAll()method has been replaced withqueryAllJobInfo(). This new method returns a map ofJobInfoobjects instead ofJobStateobjects. This change should provide more detailed information about all jobs.248-281: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [214-277]
The
queryJobStatusById()andqueryExtendedJobStateById()methods have been deprecated and replaced withqueryJobInfoByID()andqueryExtendedJobInfoByID(), respectively. These new methods returnJobInfoorExtendedJobInfoobjects instead ofJobStateobjects. This change should provide more detailed information about jobs.src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/GridEngineBasedJobManager.groovy (12)
1-13: The copyright notice and import statements look fine.
28-34: The
parseTimemethod has been overridden to convert a string to aZonedDateTimeobject. Ensure that the string passed to this method is always a valid epoch second.36-37: The constructor for
GridEngineBasedJobManagerlooks fine.58-92: The
queryJobInfomethod has been updated to return a map ofJobInfoobjects instead ofJobStateobjects. This change is in line with the PR summary. Ensure that all calls to this method have been updated to handle the new return type.94-103: The
assembleJobInfoQueryCommandmethod has been updated to return a string. This change is in line with the PR summary. Ensure that all calls to this method have been updated to handle the new return type.118-123: The
safelyCastToBufferValuemethod is a new addition. It attempts to cast an object to aBufferValue. If the cast fails, it returns null. This is a good practice for error handling.125-129: The
safelyCastToIntegermethod is a new addition. It attempts to cast an object to anInteger. If the cast fails, it returns null. This is a good practice for error handling.131-136: The
safelyCastToDurationmethod is a new addition. It attempts to cast an object to aDuration. If the cast fails, it returns null. This is a good practice for error handling.139-144: The
safelyCastToTimeUnitmethod is a new addition. It attempts to cast an object to aTimeUnit. If the cast fails, it returns null. This is a good practice for error handling.146-157: The
getExecutionHostsmethod is a new addition. It attempts to parse a string of hosts into a list of unique host names. This method seems to be well-implemented.159-169: The
getJobDependenciesmethod is a new addition. It attempts to parse a string of dependencies into a list of job IDs. This method seems to be well-implemented.171-187: The
safelyGetQstatFilemethod is a new addition. It attempts to parse a string into aFileobject. If the string is not a valid file path, it returns null. This is a good practice for error handling.src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/rest/LSFRestJobManagerSpec.groovy (1)
- 1-121: The test methods are currently placeholders and return
null. Ensure to implement the test logic for each method to validate the functionality of theLSFRestJobManagerclass.src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManagerSpec.groovy (5)
21-30: The test case "test parseColonSeparatedHHMMSSDuration, parse duration" is well written and covers different scenarios. It's good to see that edge cases are being tested.
32-39: The test case "test parseColonSeparatedHHMMSSDuration, parse duration fails" is also well written and tests the error handling of the function. It's good to see that error scenarios are being tested.
41-49: The
prepareClassLoggermethod uses reflection to set thelogfield of the specified class. This is a powerful feature, but it can also be dangerous if not used carefully. Make sure that thelogfield is intended to be changed in this way, as it could potentially affect other parts of the code that use thelogfield.51-63: The test case "test withCaughtAndLoggedException throws exception" is well written and tests the error handling of the function. It's good to see that error scenarios are being tested.
6-74: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [65-75]
The test case "test withCaughtAndLoggedException returns value" is well written and tests the normal operation of the function. It's good to see that normal scenarios are being tested.
src/test/resources/de/dkfz/roddy/execution/jobs/cluster/lsf/bjobsJobTemplatePart2.txt (1)
- 1-32: This hunk represents a job description with various fields. It seems to be a part of a test resource file. As long as the changes reflect the expected state for the test, it should be fine. However, ensure that the changes in the job description are consistent with the changes made in the codebase and the test cases.
src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParserTest.groovy (3)
6-13: The import statements have been updated to reflect the changes in the codebase. The
TestExecutionServiceis now being used instead ofBEExecutionService.25-29: The
PBSJobManagerconstructor has been updated to useTestExecutionServiceinstead ofBEExecutionService. This change is consistent with the removal of the daemon-related code from theBatchEuphoriaJobManagerclass.49-63: The assertions related to resource allocation and parameters have been modified to use the updated method names and return types. This change is consistent with the introduction of the
JobInfoandExtendedJobInfoclasses and the deprecation of thequeryJobStatusand related methods.src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManagerSpec.groovy (7)
1-5: Ensure that the copyright notice is up-to-date.
44-62: These tests are good for verifying the date/time conversion methods. However, consider adding more edge cases to ensure robustness.
68-93: These tests are good for verifying the
parseTimemethod. However, consider adding more edge cases to ensure robustness.95-107: This test verifies the
stripAwayStatusInfomethod. Consider adding more edge cases to ensure robustness.109-123: These methods are placeholders for future tests. Ensure that tests are added for these methods to verify their functionality.
125-155: These tests verify the
queryJobStateByJobandqueryJobStatesByIDmethods. Ensure that the JSON file used in the tests is up-to-date and contains the correct data.157-367: These methods are placeholders for future tests. Ensure that tests are added for these methods to verify their functionality.
src/test/resources/de/dkfz/roddy/execution/jobs/cluster/sge/simpleQStatOutput.txt (1)
- 1-6: This hunk is a test resource file that represents the output of a
qstatcommand in a Sun Grid Engine (SGE) environment. It's used to simulate the SGE environment for testing purposes. The changes are adding new job entries to the file. Ensure that the new entries are correctly formatted and that they match the expected output of theqstatcommand. Also, verify that the test cases that use this file have been updated to reflect these changes.src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/ClusterJobManager.groovy (6)
16-19: The new import statements are fine as long as they are used in the code. Ensure that they are not unused imports.
33-36: The access modifier of the
withCaughtAndLoggedExceptionmethod has been changed fromprotectedto package-private (static). This change is fine as long as it doesn't break any code that was using this method with its previous access modifier.50-58: The new method
toJobIDis fine. It converts a raw job ID string to aBEJobIDobject. However, it throws aBEExceptionif the conversion fails. Make sure that this exception is properly handled wherever this method is called.60-65: The new method
safelyParseColonSeparatedDurationis fine. It safely parses a colon-separated duration string. However, it returnsnullif the parsing fails. Make sure that the return value of this method is checked fornullwherever it is used.67-74: The new method
safelyParseTimeis fine. It safely parses a time string and returns aZonedDateTimeobject. However, it returnsnullif the parsing fails. Make sure that the return value of this method is checked fornullwherever it is used.76-76: The new abstract method
parseTimeis fine. It parses a time string and returns aZonedDateTimeobject. Make sure that this method is implemented in all subclasses ofClusterJobManager.src/main/groovy/de/dkfz/roddy/execution/jobs/direct/synchronousexecution/DirectSynchronousExecutionJobManager.groovy (4)
24-29: The
createCommandmethod is creating a newDirectCommandwith an empty list. Ensure that this is the intended behavior and that the empty list won't cause any issues.37-39: The
queryJobInfomethod is returning an empty map. Ensure that this is the intended behavior and that the empty map won't cause any issues.42-44: The
queryExtendedJobInfomethod is returning an empty map. Ensure that this is the intended behavior and that the empty map won't cause any issues.171-173: The
queryExtendedJobStateByIdmethod is returning an empty map. Ensure that this is the intended behavior and that the empty map won't cause any issues.src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFCommandParser.groovy (3)
3-19: The import statements and class documentation look fine. The class is correctly annotated with
@CompileStaticfor performance optimization in Groovy.52-62: The code checks if the command string starts with "bsub" and then splits the command string into a collection of strings. It then iterates over the collection to parse the options. This logic seems correct, but it's important to ensure that the command string is always in the expected format.
122-132: The
toGenericJobInfo()method has been updated to useExtendedJobInfoinstead ofGenericJobInfo. It creates a newExtendedJobInfoobject and sets the requested resources. The logic seems correct, but it's important to ensure that all calls to this method have been updated to handleExtendedJobInfoobjects.src/test/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManagerSpec.groovy (4)
6-24: New imports have been added to support the changes in the test suite. Ensure that these new dependencies are correctly managed and do not introduce any conflicts.
30-44: The
test convertResourceSetmethod has been added to test the conversion of resource set information. The test cases look comprehensive and cover different scenarios.46-66: The
getGenericJobInfoObjectForTestsmethod has been added to create a generic job info object for testing purposes. This method will help in reducing code duplication in the test suite.173-185: These methods test the
queryExtendedJobInfByIDmethod with different scenarios. The test cases look comprehensive and cover different scenarios.src/test/groovy/de/dkfz/roddy/execution/jobs/SubmissionCommandTest.groovy (4)
14-20: The method
queryExtendedJobStateByIdhas been renamed toqueryExtendedJobInfoand its return type has been changed fromMap<BEJobID, GenericJobInfo>toMap<BEJobID, ExtendedJobInfo>. Ensure that all calls to this method throughout the codebase have been updated to match the new signature.56-65: The method
getQueryCommandForJobInfohas been renamed togetQueryCommandForExtendedJobInfo. Ensure that all calls to this method throughout the codebase have been updated to match the new name.71-75: The return type of the method
parseGenericJobInfohas been changed fromGenericJobInfotoExtendedJobInfo. Ensure that all calls to this method throughout the codebase have been updated to match the new return type.101-109: The method
queryJobStateshas been renamed toqueryJobInfoand a new methodqueryExtendedJobInfohas been added. Ensure that all calls to these methods throughout the codebase have been updated to match the new names and signatures.src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSCommandParser.groovy (2)
9-13: The import statement for
GenericJobInfohas been replaced withExtendedJobInfo. This change is consistent with the PR summary and seems to be part of the refactoring effort to replaceGenericJobInfowithExtendedJobInfo. Ensure that all references toGenericJobInfoin the codebase have been updated accordingly.131-138: The method
toGenericJobInfo()has been renamed totoGenericJobInfo()and now creates an instance ofExtendedJobInfoinstead ofGenericJobInfo. The methodsetAskedResources()has been renamed tosetRequestedResources(). These changes are consistent with the PR summary and seem to be part of the refactoring effort to replaceGenericJobInfowithExtendedJobInfoand to renamesetAskedResources()tosetRequestedResources(). Ensure that all calls to these methods in the codebase have been updated accordingly.- GenericJobInfo toGenericJobInfo() { - GenericJobInfo jInfo = new GenericJobInfo(jobName, script, jobID, parameters, dependencies) - ResourceSet askedResources = new ResourceSet(null, memory ? new BufferValue(memory as Integer, bufferUnit) : null, - cores ? cores as Integer : null, nodes ? nodes as Integer : null, walltime ? new TimeUnit(walltime) : null, - null, null, null) - jInfo.setAskedResources(askedResources) - return jInfo - } + ExtendedJobInfo toGenericJobInfo() { + ExtendedJobInfo jInfo = new ExtendedJobInfo(jobName, script, jobID, parameters, dependencies) + ResourceSet askedResources = new ResourceSet(null, memory ? new BufferValue(memory as Integer, bufferUnit) : null, + cores ? cores as Integer : null, nodes ? nodes as Integer : null, walltime ? new TimeUnit(walltime) : null, + null, null, null) + jInfo.setRequestedResources(askedResources) + return jInfo + }src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/pbs/PBSJobManager.groovy (5)
7-22: The import statements look fine. No unused imports detected.
36-41: The
parseGenericJobInfofunction has been updated to returnExtendedJobInfoinstead ofJobInfo. Ensure that all calls to this function throughout the codebase have been updated to handle the new return type.81-92: The function names
getQueryJobStatesCommandandgetExtendedQueryJobStatesCommandhave been renamed togetQueryCommandForJobInfoandgetQueryCommandForExtendedJobInforespectively. This is a good change as the new names are more descriptive and align better with the function's purpose.163-184: The new function
queryExtendedJobInfoqueries extended job information. It seems to be correctly implemented. However, the comment on lines 168-171 suggests that there might be some confusion about whether to include the-u $userIDForQueriesoption in theqStatCommand. This option would limit the query to jobs belonging to a specific user. If you want to query all jobs regardless of the user, then you should not include this option.191-251: The new function
processQstatOutputFromXMLNodeChildprocesses the qstat output and createsExtendedJobInfoobjects. The function appears to be correctly implemented. However, it's a bit long and complex. Consider breaking it down into smaller, more manageable functions. This would improve readability and maintainability.src/main/groovy/de/dkfz/roddy/execution/jobs/cluster/lsf/LSFJobManager.groovy (10)
1-5: The copyright information has been updated to 2019. Please ensure that this is the correct year for the changes made.
32-37: The LSF command strings have been updated. Ensure that these commands are correct and compatible with the LSF version in use.
64-93: The
parseTimemethod has been updated to handle different time formats reported by LSF. This is a good improvement for flexibility and robustness. However, it's important to ensure that all possible time formats are covered and that the method behaves as expected in all cases.100-107: The
stripAwayStatusInfomethod has been added to remove status information from time strings. This is a good addition for parsing LSF time strings. However, it's important to ensure that this method behaves as expected in all cases.132-134: The
parseGenericJobInfomethod has been renamed toparseExtendedJobInfoand now returns anExtendedJobInfoobject. This is a good change for clarity and consistency. However, ensure that all calls to this method throughout the codebase have been updated to match the new name and return type.137-148: The
queryJobStatesmethod has been renamed toqueryJobInfoand now returns a map ofJobInfoobjects. This is a good change for clarity and consistency. However, ensure that all calls to this method throughout the codebase have been updated to match the new name and return type.150-158: The
queryExtendedJobInfomethod has been added to query extended job information. This is a good addition for retrieving more detailed job information. However, ensure that this method behaves as expected in all cases.171-274: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [160-180]
The
runBjobsmethod has been updated to handle extended job states. This is a good improvement for flexibility and robustness. However, it's important to ensure that this method behaves as expected in all cases.
201-260: The
convertJobDetailsMapToGenericJobInfoObjectmethod has been added to convert a map of job details to aGenericJobInfoobject. This is a good addition for creatingGenericJobInfoobjects from raw job details. However, ensure that this method behaves as expected in all cases.262-272: The
safelyCastToBufferValuemethod has been added to safely cast a string to aBufferValue. This is a good addition for handling potential casting errors. However, ensure that this method behaves as expected in all cases.
| /* | ||
| * Copyright (c) 2019 German Cancer Research Center (Deutsches Krebsforschungszentrum, DKFZ). | ||
| * | ||
| * Distributed under the MIT License (license terms are at https://www.github.com/TheRoddyWMS/BatchEuphoria/LICENSE.txt). | ||
| */ | ||
|
|
||
| package de.dkfz.roddy.execution.jobs | ||
|
|
||
| import groovy.transform.CompileStatic | ||
| import groovy.transform.EqualsAndHashCode | ||
|
|
||
| import java.time.ZonedDateTime | ||
|
|
||
| /** | ||
| * The class contains the bare minimum of information about a job gathered by | ||
| * BatchEuphoriaJobManager.queryJobInfo() | ||
| * Note that all fields except jobID can be undefined / null! | ||
| * | ||
| * There are three different settings for the content of this class: | ||
| * - A job was not found by the manager, the state will be set to UNKNOWN, with no endTime | ||
| * - A job was found but is still running, the state will be set accordingly, with no endTime | ||
| * - A job was found and is finished, the state will be set accordingly with the endTime set | ||
| * | ||
| * This class is a value class. Normally, we'd use final fields for all values or some kind of | ||
| * builder interface to create instances of it. But due to the sheer size of ExtendedJobInfo, | ||
| * we will refrain from both. Please make sure, that you do net mess things up. | ||
| */ | ||
| @CompileStatic | ||
| @EqualsAndHashCode | ||
| class JobInfo { | ||
|
|
||
| BEJobID jobID | ||
|
|
||
| /** | ||
| * The date-time the job was completed. | ||
| * Can be null, e.g. if the batch system cannot track the job anymore. | ||
| * */ | ||
| ZonedDateTime endTime | ||
|
|
||
| JobState jobState = JobState.UNKNOWN | ||
|
|
||
| JobInfo(BEJobID jobID) { | ||
| this.jobID = jobID | ||
| } | ||
| } |
There was a problem hiding this comment.
The JobInfo class is well-structured and follows good practices. It uses @CompileStatic and @EqualsAndHashCode annotations, which are good for performance and object comparison respectively. The class is also well-documented. However, consider making the jobID field final since it's set only once in the constructor and should not be changed afterwards. This will make the class more robust and prevent potential bugs.
- BEJobID jobID
+ final BEJobID jobIDCommitable suggestion (Beta)
| /* | |
| * Copyright (c) 2019 German Cancer Research Center (Deutsches Krebsforschungszentrum, DKFZ). | |
| * | |
| * Distributed under the MIT License (license terms are at https://www.github.com/TheRoddyWMS/BatchEuphoria/LICENSE.txt). | |
| */ | |
| package de.dkfz.roddy.execution.jobs | |
| import groovy.transform.CompileStatic | |
| import groovy.transform.EqualsAndHashCode | |
| import java.time.ZonedDateTime | |
| /** | |
| * The class contains the bare minimum of information about a job gathered by | |
| * BatchEuphoriaJobManager.queryJobInfo() | |
| * Note that all fields except jobID can be undefined / null! | |
| * | |
| * There are three different settings for the content of this class: | |
| * - A job was not found by the manager, the state will be set to UNKNOWN, with no endTime | |
| * - A job was found but is still running, the state will be set accordingly, with no endTime | |
| * - A job was found and is finished, the state will be set accordingly with the endTime set | |
| * | |
| * This class is a value class. Normally, we'd use final fields for all values or some kind of | |
| * builder interface to create instances of it. But due to the sheer size of ExtendedJobInfo, | |
| * we will refrain from both. Please make sure, that you do net mess things up. | |
| */ | |
| @CompileStatic | |
| @EqualsAndHashCode | |
| class JobInfo { | |
| BEJobID jobID | |
| /** | |
| * The date-time the job was completed. | |
| * Can be null, e.g. if the batch system cannot track the job anymore. | |
| * */ | |
| ZonedDateTime endTime | |
| JobState jobState = JobState.UNKNOWN | |
| JobInfo(BEJobID jobID) { | |
| this.jobID = jobID | |
| } | |
| } | |
| /* | |
| * Copyright (c) 2019 German Cancer Research Center (Deutsches Krebsforschungszentrum, DKFZ). | |
| * | |
| * Distributed under the MIT License (license terms are at https://www.github.com/TheRoddyWMS/BatchEuphoria/LICENSE.txt). | |
| */ | |
| package de.dkfz.roddy.execution.jobs | |
| import groovy.transform.CompileStatic | |
| import groovy.transform.EqualsAndHashCode | |
| import java.time.ZonedDateTime | |
| /** | |
| * The class contains the bare minimum of information about a job gathered by | |
| * BatchEuphoriaJobManager.queryJobInfo() | |
| * Note that all fields except jobID can be undefined / null! | |
| * | |
| * There are three different settings for the content of this class: | |
| * - A job was not found by the manager, the state will be set to UNKNOWN, with no endTime | |
| * - A job was found but is still running, the state will be set accordingly, with no endTime | |
| * - A job was found and is finished, the state will be set accordingly with the endTime set | |
| * | |
| * This class is a value class. Normally, we'd use final fields for all values or some kind of | |
| * builder interface to create instances of it. But due to the sheer size of ExtendedJobInfo, | |
| * we will refrain from both. Please make sure, that you do net mess things up. | |
| */ | |
| @CompileStatic | |
| @EqualsAndHashCode | |
| class JobInfo { | |
| final BEJobID jobID | |
| /** | |
| * The date-time the job was completed. | |
| * Can be null, e.g. if the batch system cannot track the job anymore. | |
| * */ | |
| ZonedDateTime endTime | |
| JobState jobState = JobState.UNKNOWN | |
| JobInfo(BEJobID jobID) { | |
| this.jobID = jobID | |
| } | |
| } |
| static final File getResourceFile(Class _class, String file) { | ||
| String subDir = _class.package.name.replace(".", "/") | ||
| new File("src/test/resources/${subDir}/", file) | ||
| } |
There was a problem hiding this comment.
The method getResourceFile assumes that the resource file is always located in the src/test/resources directory. This might not always be the case, especially if the tests are run from a different directory or in a different environment. Consider using ClassLoader.getResource or ClassLoader.getResourceAsStream to get a URL or InputStream for the resource file. This will work regardless of where the resource file is located.
- new File("src/test/resources/${subDir}/", file)
+ _class.classLoader.getResource("${subDir}/${file}")Commitable suggestion (Beta)
| static final File getResourceFile(Class _class, String file) { | |
| String subDir = _class.package.name.replace(".", "/") | |
| new File("src/test/resources/${subDir}/", file) | |
| } | |
| static final File getResourceFile(Class _class, String file) { | |
| String subDir = _class.getPackage().getName().replace(".", "/"); | |
| return new File(_class.getClassLoader().getResource(subDir + "/" + file).getFile()); | |
| } |
| def "test getResourceFile"() { | ||
| when: | ||
| File resourceFile = getResourceFile(BETestBaseSpec, "getResourceFileTest.txt") | ||
| String text = resourceFile.text | ||
|
|
||
| then: | ||
| resourceFile.exists() | ||
| text == "abc" | ||
| } |
There was a problem hiding this comment.
The test case test getResourceFile is tightly coupled with the actual file content ("abc"). If the file content changes, the test will fail. Consider checking only if the file exists and is readable, which is the actual responsibility of the getResourceFile method.
- text == "abc"
+ resourceFile.canRead()Commitable suggestion (Beta)
| def "test getResourceFile"() { | |
| when: | |
| File resourceFile = getResourceFile(BETestBaseSpec, "getResourceFileTest.txt") | |
| String text = resourceFile.text | |
| then: | |
| resourceFile.exists() | |
| text == "abc" | |
| } | |
| def "test getResourceFile"() { | |
| when: | |
| File resourceFile = getResourceFile(BETestBaseSpec, "getResourceFileTest.txt") | |
| String text = resourceFile.text | |
| then: | |
| resourceFile.exists() | |
| resourceFile.canRead() | |
| } |
| Map<BEJob, JobState> queryJobStatus(List<BEJob> jobs) { | ||
| queryJobInfoByJob(jobs).collectEntries { BEJob job, JobInfo ji -> [job, ji.jobState] } as Map<BEJob, JobState> | ||
| } | ||
|
|
||
| void stopUpdateDaemon() { | ||
| updateDaemonShallBeClosed = true | ||
| updateDaemonThread?.join() | ||
| /** | ||
| * Queries the status of all jobs in the list. | ||
| * | ||
| * Every job ID in the list is supposed to have an entry in the result map. If | ||
| * the manager cannot retrieve info about the job, the result will be UNKNOWN | ||
| * for this particular job. | ||
| * | ||
| * @param jobIds | ||
| * @return | ||
| */ | ||
| @Deprecated | ||
| Map<BEJobID, JobState> queryJobStatusById(List<BEJobID> jobIds) { | ||
| queryJobInfoByID(jobIds).collectEntries { BEJobID id, JobInfo ji -> [id, ji.jobState] } as Map<BEJobID, JobState> | ||
| } | ||
|
|
||
| private void updateActiveJobList() { | ||
| List<BEJobID> listOfRemovableJobs = [] | ||
| synchronized (activeJobs) { | ||
| Map<BEJobID, JobState> states = queryJobStatesUsingCache(activeJobs.keySet() as List<BEJobID>, true) | ||
|
|
||
| for (BEJobID id : activeJobs.keySet()) { | ||
| JobState jobState = states.get(id) | ||
| BEJob job = activeJobs.get(id) | ||
|
|
||
| job.setJobState(jobState) | ||
| if (!jobState.isPlannedOrRunning()) { | ||
| synchronized (updateDaemonListeners) { | ||
| updateDaemonListeners.each { it.jobEnded(job, jobState) } | ||
| } | ||
| if (!jobState.successful) { | ||
| surveilledJobsHadErrors = true | ||
| } | ||
| listOfRemovableJobs << id | ||
| } | ||
| } | ||
| listOfRemovableJobs.each { activeJobs.remove(it) } | ||
| } | ||
| /** | ||
| * Queries the status of all jobs. | ||
| * | ||
| * @return | ||
| */ | ||
| @Deprecated | ||
| Map<BEJobID, JobState> queryJobStatusAll(boolean forceUpdate = false) { | ||
| return queryAllJobInfo().collectEntries { BEJobID id, JobInfo ji -> [id, ji.jobState] } as Map<BEJobID, JobState> | ||
| } | ||
|
|
||
| private Map<BEJobID, JobState> queryJobStatesUsingCache(List<BEJobID> jobIDs, boolean forceUpdate) { | ||
| if (forceUpdate || lastCacheUpdate == null || cacheUpdateInterval == Duration.ZERO || | ||
| Duration.between(lastCacheUpdate, LocalDateTime.now()) > cacheUpdateInterval) { | ||
| synchronized (cacheStatesLock) { | ||
| cachedStates = queryJobStates(jobIDs) | ||
| } | ||
| lastCacheUpdate = LocalDateTime.now() | ||
| } | ||
| return new HashMap(cachedStates) | ||
| @Deprecated | ||
| Map<BEJob, ExtendedJobInfo> queryExtendedJobState(List<BEJob> jobs) { | ||
| queryExtendedJobInfoByJob(jobs) | ||
| } | ||
|
|
||
| /** | ||
| * The method will wait until all started jobs are finished (with or without errors). | ||
| * | ||
| * Note, that the method does not allow further job submission! As soon, as you call it, you cannot submit jobs! | ||
| * Will be used to gather extended information about a job like: | ||
| * - The used memory | ||
| * - The used cores | ||
| * - The used walltime | ||
| * | ||
| * @return true, if there were NO errors, false, if there were any. | ||
| * @param jobIds | ||
| * @return | ||
| */ | ||
| boolean waitForJobsToFinish() { | ||
| if (!updateDaemonThread) { | ||
| throw new BEException("The job manager must be created with JobManagerOption.createDaemon set to true to make waitForJobsToFinish() work.") | ||
| } | ||
| forbidFurtherJobSubmission = true | ||
| while (!updateDaemonShallBeClosed) { | ||
| synchronized (activeJobs) { | ||
| if (activeJobs.isEmpty()) { | ||
| break | ||
| } | ||
| } | ||
| waitForUpdateIntervalDuration() | ||
| } | ||
| return !surveilledJobsHadErrors | ||
| @Deprecated | ||
| Map<BEJobID, ExtendedJobInfo> queryExtendedJobStateById(List<BEJobID> jobIds) { | ||
| return queryExtendedJobInfo(jobIds) | ||
| } | ||
|
|
There was a problem hiding this comment.
The deprecated methods queryJobStatus(), queryJobStatusById(), queryJobStatusAll(), queryExtendedJobState(), and queryExtendedJobStateById() are still present in the code. If these methods are no longer needed, consider removing them to clean up the codebase.
| package de.dkfz.roddy.execution.jobs | ||
|
|
||
| import spock.lang.Specification | ||
|
|
||
| class ExtendedJobInfoTest extends Specification { | ||
| def "test @EqualsAndHashCode annotation with differing base class"() { | ||
| when: | ||
| def g1 = new ExtendedJobInfo(new BEJobID("1000")) | ||
| def g2 = new ExtendedJobInfo(new BEJobID("1001")) | ||
|
|
||
| then: | ||
| g1 != g2 | ||
| } | ||
| } |
There was a problem hiding this comment.
The test case is well written and checks the behavior of the @EqualsAndHashCode annotation correctly. It's good to see that you are testing edge cases like this. However, it would be beneficial to add more test cases to cover other scenarios, such as when the BEJobID instances are the same, or when other properties of ExtendedJobInfo are different.
def "test @EqualsAndHashCode annotation with same base class"() {
when:
def g1 = new ExtendedJobInfo(new BEJobID("1000"))
def g2 = new ExtendedJobInfo(new BEJobID("1000"))
then:
g1 == g2
}
def "test @EqualsAndHashCode annotation with different properties"() {
when:
def g1 = new ExtendedJobInfo(new BEJobID("1000"))
g1.someProperty = "value1"
def g2 = new ExtendedJobInfo(new BEJobID("1000"))
g2.someProperty = "value2"
then:
g1 != g2
}Please replace someProperty with an actual property of ExtendedJobInfo.
| String getQueryCommandForExtendedJobInfo() { | ||
| return null | ||
| } |
There was a problem hiding this comment.
The getQueryCommandForExtendedJobInfo method is returning null. This could potentially lead to NullPointerExceptions if the return value is used without null checks. Consider returning an Optional or a default string.
- return null
+ return Optional.empty()Commitable suggestion (Beta)
| String getQueryCommandForExtendedJobInfo() { | |
| return null | |
| } | |
| String getQueryCommandForExtendedJobInfo() { | |
| return Optional.empty(); | |
| } |
| return null | ||
| } | ||
|
|
||
| void "processQstatOutput, replace placeholder PBS_JOBID in logFile and errorLogFile with job id "() { | ||
| given: | ||
| String rawXMLOutput=''' | ||
| <Data> | ||
| <Job> | ||
| <Job_Id>15976927.testServer</Job_Id> | ||
| <Output_Path>/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o$PBS_JOBID</Output_Path> | ||
| <Error_Path>tbi-pbs:/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e$PBS_JOBID</Error_Path> | ||
| </Job> | ||
| </Data> | ||
| ''' | ||
| def "test getDefaultForHoldJobsEnabled"() { | ||
| return null | ||
| } | ||
|
|
||
| when: | ||
| Map<BEJobID, GenericJobInfo> jobInfo = manager.processQstatOutputFromXML(rawXMLOutput) | ||
| def "test isHoldJobsEnabled"() { | ||
| return null | ||
| } | ||
|
|
||
| then: | ||
| jobInfo.size() == 1 | ||
| jobInfo.get(new BEJobID("15976927")).logFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o15976927.testServer" | ||
| jobInfo.get(new BEJobID("15976927")).errorLogFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e15976927.testServer" | ||
| def "test getUserEmail"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getUserMask"() { | ||
| return null | ||
| } | ||
|
|
||
| void "parseColonSeparatedHHMMSSDuration, parse duration"() { | ||
| def "test getUserGroup"() { | ||
| return null | ||
| } | ||
|
|
||
| given: | ||
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | ||
| method.setAccessible(true) | ||
| def "test getUserAccount"() { | ||
| return null | ||
| } | ||
|
|
||
| expect: | ||
| parsedDuration == method.invoke(null, input) | ||
| def "test executesWithoutJobSystem"() { | ||
| return null | ||
| } | ||
|
|
||
| where: | ||
| input || parsedDuration | ||
| "00:00:00" || Duration.ofSeconds(0) | ||
| "24:00:00" || Duration.ofHours(24) | ||
| "119:00:00" || Duration.ofHours(119) | ||
| def "test convertResourceSet"() { | ||
| return null | ||
| } | ||
|
|
||
|
|
||
| void "parseColonSeparatedHHMMSSDuration, parse duration fails"() { | ||
| def "test collectJobIDsFromJobs"() { | ||
| return null | ||
| } | ||
|
|
||
| given: | ||
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | ||
| method.setAccessible(true) | ||
| def "test extractAndSetJobResultFromExecutionResult"() { | ||
| return null | ||
| } | ||
|
|
||
| when: | ||
| method.invoke(null, "02:42") | ||
| def "test createCommand"() { | ||
| return null | ||
| } | ||
|
|
||
| then: | ||
| InvocationTargetException e = thrown(InvocationTargetException) | ||
| e.targetException.message == "Duration string is not of the format HH+:MM:SS: '02:42'" | ||
| def "test parseJobID"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test parseJobState"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test executeStartHeldJobs"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test executeKillJobs"() { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Several new test methods have been added but are not implemented yet. These methods should be implemented to ensure comprehensive testing of the PBSJobManager class.
- def "test submitJob"() {
- return null
- }
+ def "test submitJob"() {
+ // Implement test logic here
+ }Commitable suggestion (Beta)
| def "test submitJob"() { | |
| return null | |
| } | |
| def "test addToListOfStartedJobs"() { | |
| return null | |
| } | |
| def "test startHeldJobs"() { | |
| return null | |
| } | |
| def "test killJobs"() { | |
| return null | |
| } | |
| def "test queryJobStateByJob"() { | |
| return null | |
| } | |
| def "test queryJobStateByID"() { | |
| return null | |
| } | |
| def "test queryJobInfByJob"() { | |
| return null | |
| } | |
| def "test queryJobInfByID"() { | |
| return null | |
| } | |
| def "test queryAllJobInf"() { | |
| return null | |
| } | |
| def "test queryExtendedJobInfByJob"() { | |
| return null | |
| } | |
| def "test queryExtendedJobInfByID"() { | |
| return null | |
| } | |
| void "test queryExtendedJobInfByID with queued job"() { | |
| given: | |
| def jobManager = createJobManagerWithModifiedExecutionService("processExtendedQstatXMLOutputWithQueuedJob.xml") | |
| ExtendedJobInfo expected = getGenericJobInfoObjectForTests() | |
| when: | |
| Map<BEJobID, GenericJobInfo> result = manager.processQstatOutputFromXML(outputQueued) | |
| Map<BEJobID, ExtendedJobInfo> result = jobManager.queryExtendedJobInfo([testJobID]) | |
| ExtendedJobInfo jobInfo = result.get(testJobID) | |
| then: | |
| result.size() == 1 | |
| GenericJobInfo jobInfo = result.get(new BEJobID("4499334")) | |
| jobInfo | |
| jobInfo.askedResources.size == null | |
| jobInfo.askedResources.mem == new BufferValue(37888, BufferUnit.M) | |
| jobInfo.askedResources.cores == 4 | |
| jobInfo.askedResources.nodes == 1 | |
| jobInfo.askedResources.walltime == Duration.ofHours(5) | |
| jobInfo.askedResources.storage == null | |
| jobInfo.askedResources.queue == "otp" | |
| jobInfo.askedResources.nthreads == null | |
| jobInfo.askedResources.swap == null | |
| jobInfo.usedResources.size == null | |
| jobInfo.usedResources.mem == null | |
| jobInfo.usedResources.cores == null | |
| jobInfo.usedResources.nodes == null | |
| jobInfo.usedResources.walltime == null | |
| jobInfo.usedResources.storage == null | |
| jobInfo.usedResources.queue == "otp" | |
| jobInfo.usedResources.nthreads == null | |
| jobInfo.usedResources.swap == null | |
| jobInfo.jobName == "r180328_183957634_pid_4_starAlignment" | |
| jobInfo.tool == null | |
| jobInfo.jobID == new BEJobID("4499334") | |
| jobInfo.submitTime.isEqual ZonedDateTime.of(2018, 03, 28, 16, 39, 22, 0, ZoneOffset.UTC) | |
| jobInfo.eligibleTime.isEqual ZonedDateTime.of(2018, 03, 28, 16, 39, 22, 0, ZoneOffset.UTC) | |
| jobInfo.startTime == null | |
| jobInfo.endTime == null | |
| jobInfo.executionHosts == null | |
| jobInfo.submissionHost == "subm.example.com" | |
| jobInfo.priority == "0" | |
| jobInfo.logFile == new File("/net/isilon/otp/workflow-tests/tmp/RnaPairedAlignmentWorkflow-otp-web-2018-03-28-18-36-57-153+0200-mFrFsCEXnGMdFEGC/root_path/projectDirName_20/sequencing/rna_sequencing/view-by-pid/pid_4/control/paired/merged-alignment/.merging_0/roddyExecutionStore/exec_180328_183957634_otp-data_RNA/r180328_183957634_pid_4_starAlignment.o4499334.pbsserver") | |
| jobInfo.errorLogFile == new File("/net/isilon/otp/workflow-tests/tmp/RnaPairedAlignmentWorkflow-otp-web-2018-03-28-18-36-57-153+0200-mFrFsCEXnGMdFEGC/root_path/projectDirName_20/sequencing/rna_sequencing/view-by-pid/pid_4/control/paired/merged-alignment/.merging_0/roddyExecutionStore/exec_180328_183957634_otp-data_RNA/r180328_183957634_pid_4_starAlignment.o4499334.pbsserver") | |
| jobInfo.inputFile == null | |
| jobInfo.user == null | |
| jobInfo.userGroup == null | |
| jobInfo.resourceReq == '-v TOOL_ID=starAlignment,PARAMETER_FILE=/net/isilon/otp/workflow-tests/tmp/RnaPairedAlignmentWorkflow-otp-web-2018-03-28-18-36-57-153+0200-mFrFsCEXnGMdFEGC/root_path/projectDirName_20/sequencing/rna_sequencing/view-by-pid/pid_4/control/paired/merged-alignment/.merging_0/roddyExecutionStore/exec_180328_183957634_otp-data_RNA/r180328_183957634_pid_4_starAlignment_3.parameters,CONFIG_FILE=/net/isilon/otp/workflow-tests/tmp/RnaPairedAlignmentWorkflow-otp-web-2018-03-28-18-36-57-153+0200-mFrFsCEXnGMdFEGC/root_path/projectDirName_20/sequencing/rna_sequencing/view-by-pid/pid_4/control/paired/merged-alignment/.merging_0/roddyExecutionStore/exec_180328_183957634_otp-data_RNA/r180328_183957634_pid_4_starAlignment_3.parameters,debugWrapInScript=false,baseEnvironmentScript=/tbi/software/sourceme -N r180328_183957634_pid_4_starAlignment -h -w /home/otp-data -j oe -o /net/isilon/otp/workflow-tests/tmp/RnaPairedAlignmentWorkflow-otp-web-2018-03-28-18-36-57-153+0200-mFrFsCEXnGMdFEGC/root_path/projectDirName_20/sequencing/rna_sequencing/view-by-pid/pid_4/control/paired/merged-alignment/.merging_0/roddyExecutionStore/exec_180328_183957634_otp-data_RNA/r180328_183957634_pid_4_starAlignment.o$PBS_JOBID -l mem=37888M -l walltime=00:05:00:00 -l nodes=1:ppn=4 -q otp /net/isilon/otp/workflow-tests/tmp/RnaPairedAlignmentWorkflow-otp-web-2018-03-28-18-36-57-153+0200-mFrFsCEXnGMdFEGC/root_path/projectDirName_20/sequencing/rna_sequencing/view-by-pid/pid_4/control/paired/merged-alignment/.merging_0/roddyExecutionStore/exec_180328_183957634_otp-data_RNA/analysisTools/roddyTools/wrapInScript.sh' | |
| jobInfo.startCount == null | |
| jobInfo.account == null | |
| jobInfo.server == "pbsserver" | |
| jobInfo.umask == null | |
| jobInfo.parameters == null | |
| jobInfo.parentJobIDs == [] | |
| jobInfo.otherSettings == null | |
| jobInfo.jobState == JobState.QUEUED | |
| jobInfo.userTime == null | |
| jobInfo.systemTime == null | |
| jobInfo.pendReason == null | |
| jobInfo.execHome == null | |
| jobInfo.execUserName == null | |
| jobInfo.pidStr == null | |
| jobInfo.pgidStr == null | |
| jobInfo.exitCode == null | |
| jobInfo.jobGroup == null | |
| jobInfo.description == null | |
| jobInfo.execCwd == null //??? | |
| jobInfo.askedHostsStr == null | |
| jobInfo.cwd == null | |
| jobInfo.projectName == null | |
| jobInfo.cpuTime == null | |
| jobInfo.runTime == null | |
| jobInfo.timeUserSuspState == null | |
| jobInfo.timePendState == null | |
| jobInfo.timePendSuspState == null | |
| jobInfo.timeSystemSuspState == null | |
| jobInfo.timeUnknownState == null | |
| jobInfo.timeOfCalculation == null | |
| } | |
| void "test processQstatOutputFromXML with finished job, with newline"() { | |
| jobInfo.requestedResources == expected.requestedResources | |
| jobInfo.usedResources == expected.usedResources | |
| jobInfo.submitTime == expected.submitTime | |
| jobInfo == expected | |
| } | |
| void "test queryExtendedJobInfByID with finished job"() { | |
| given: | |
| def jobManager = createJobManagerWithModifiedExecutionService("processExtendedQstatXMLOutputWithFinishedJob.xml") | |
| ExtendedJobInfo expected = getGenericJobInfoObjectForTests() | |
| expected.usedResources = new ResourceSet(null, new BufferValue(6064, BufferUnit.k), null, null, Duration.ofSeconds(6), null, "otp", null) | |
| expected.startTime = ZonedDateTime.of(2018, 4, 5, 16, 39, 43, 0, ZoneId.systemDefault()) | |
| expected.endTime = ZonedDateTime.of(2018, 4, 5, 16, 39, 54, 0, ZoneId.systemDefault()) | |
| expected.executionHosts = ["exec-host"] | |
| expected.startCount = 1 | |
| expected.parentJobIDs = ["6059780", "6059781"] | |
| expected.jobState = JobState.COMPLETED_UNKNOWN | |
| expected.exitCode = 0 | |
| expected.cpuTime = Duration.ZERO | |
| expected.runTime = Duration.ofSeconds(10) | |
| when: | |
| Map<BEJobID, GenericJobInfo> result = manager.processQstatOutputFromXML(outputFinished) | |
| Map<BEJobID, ExtendedJobInfo> result = jobManager.queryExtendedJobInfo([testJobID]) | |
| ExtendedJobInfo jobInfo = result.get(testJobID) | |
| then: | |
| result.size() == 1 | |
| GenericJobInfo jobInfo = result.get(new BEJobID("4564045")) | |
| jobInfo | |
| jobInfo.askedResources.size == null | |
| jobInfo.askedResources.mem == new BufferValue(1024, BufferUnit.M) | |
| jobInfo.askedResources.cores == 3 | |
| jobInfo.askedResources.nodes == 1 | |
| jobInfo.askedResources.walltime == Duration.ofHours(4) | |
| jobInfo.askedResources.storage == null | |
| jobInfo.askedResources.queue == "otp" | |
| jobInfo.askedResources.nthreads == null | |
| jobInfo.askedResources.swap == null | |
| jobInfo.usedResources.size == null | |
| jobInfo.usedResources.mem == new BufferValue(6064, BufferUnit.k) | |
| jobInfo.usedResources.cores == null | |
| jobInfo.usedResources.nodes == null | |
| jobInfo.usedResources.walltime == Duration.ofSeconds(6) | |
| jobInfo.usedResources.storage == null | |
| jobInfo.usedResources.queue == "otp" | |
| jobInfo.usedResources.nthreads == null | |
| jobInfo.usedResources.swap == null | |
| jobInfo.jobName == "r180405_163953553_stds_snvJoinVcfFiles" | |
| jobInfo.tool == null | |
| jobInfo.jobID == new BEJobID("4564045") | |
| jobInfo.submitTime.isEqual ZonedDateTime.of(2018, 4, 5, 14, 39, 18, 0, ZoneOffset.UTC) | |
| jobInfo.eligibleTime.isEqual ZonedDateTime.of(2018, 4, 5, 14, 39, 42, 0, ZoneOffset.UTC) | |
| jobInfo.startTime.isEqual ZonedDateTime.of(2018, 4, 5, 14, 39, 43, 0, ZoneOffset.UTC) | |
| jobInfo.endTime.isEqual ZonedDateTime.of(2018, 4, 5, 14, 39, 54, 0, ZoneOffset.UTC) | |
| jobInfo.executionHosts == ["denbi5-int"] | |
| jobInfo.submissionHost == "subm.example.com" | |
| jobInfo.priority == "0" | |
| jobInfo.logFile == new File("/net/isilon/otp/workflow-tests/tmp/RoddyBamFileWgsRoddySnvWorkflow-otp-web-2018-04-05-16-38-05-094+0200-k4FXdCscdyo3vAxl/root_path/projectDirName_22/sequencing/whole_genome_sequencing/view-by-pid/stds/snv_results/paired/sampletypename-24_sampletypename-43/results_SNVCallingWorkflow-1.2.166-1_v1_0_2018-04-05_16h39_+0200/roddyExecutionStore/exec_180405_163953553_otp-data_WGS/r180405_163953553_stds_snvJoinVcfFiles.o4564045.pbsserver") | |
| jobInfo.errorLogFile == new File("/net/isilon/otp/workflow-tests/tmp/RoddyBamFileWgsRoddySnvWorkflow-otp-web-2018-04-05-16-38-05-094+0200-k4FXdCscdyo3vAxl/root_path/projectDirName_22/sequencing/whole_genome_sequencing/view-by-pid/stds/snv_results/paired/sampletypename-24_sampletypename-43/results_SNVCallingWorkflow-1.2.166-1_v1_0_2018-04-05_16h39_+0200/roddyExecutionStore/exec_180405_163953553_otp-data_WGS/r180405_163953553_stds_snvJoinVcfFiles.o4564045.pbsserver") | |
| jobInfo.inputFile == null | |
| jobInfo.user == null | |
| jobInfo.userGroup == null | |
| jobInfo.resourceReq == '-v TOOL_ID=snvJoinVcfFiles,PARAMETER_FILE=/net/isilon/otp/workflow-tests/tmp/RoddyBamFileWgsRoddySnvWorkflow-otp-web-2018-04-05-16-38-05-094+0200-k4FXdCscdyo3vAxl/root_path/projectDirName_22/sequencing/whole_genome_sequencing/view-by-pid/stds/snv_results/paired/sampletypename-24_sampletypename-43/results_SNVCallingWorkflow-1.2.166-1_v1_0_2018-04-05_16h39_+0200/roddyExecutionStore/exec_180405_163953553_otp-data_WGS/r180405_163953553_stds_snvJoinVcfFiles_9.parameters,CONFIG_FILE=/net/isilon/otp/workflow-tests/tmp/RoddyBamFileWgsRoddySnvWorkflow-otp-web-2018-04-05-16-38-05-094+0200-k4FXdCscdyo3vAxl/root_path/projectDirName_22/sequencing/whole_genome_sequencing/view-by-pid/stds/snv_results/paired/sampletypename-24_sampletypename-43/results_SNVCallingWorkflow-1.2.166-1_v1_0_2018-04-05_16h39_+0200/roddyExecutionStore/exec_180405_163953553_otp-data_WGS/r180405_163953553_stds_snvJoinVcfFiles_9.parameters,debugWrapInScript=false,baseEnvironmentScript=/tbi/software/sourceme -N r180405_163953553_stds_snvJoinVcfFiles -h -w /home/otp-data -j oe -o /net/isilon/otp/workflow-tests/tmp/RoddyBamFileWgsRoddySnvWorkflow-otp-web-2018-04-05-16-38-05-094+0200-k4FXdCscdyo3vAxl/root_path/projectDirName_22/sequencing/whole_genome_sequencing/view-by-pid/stds/snv_results/paired/sampletypename-24_sampletypename-43/results_SNVCallingWorkflow-1.2.166-1_v1_0_2018-04-05_16h39_+0200/roddyExecutionStore/exec_180405_163953553_otp-data_WGS/r180405_163953553_stds_snvJoinVcfFiles.o$PBS_JOBID -l mem=1024M -l walltime=00:04:00:00 -l nodes=1:ppn=3 -q otp -W depend=afterok:6059780:6059781 /net/isilon/otp/workflow-tests/tmp/RoddyBamFileWgsRoddySnvWorkflow-otp-web-2018-04-05-16-38-05-094+0200-k4FXdCscdyo3vAxl/root_path/projectDirName_22/sequencing/whole_genome_sequencing/view-by-pid/stds/snv_results/paired/sampletypename-24_sampletypename-43/results_SNVCallingWorkflow-1.2.166-1_v1_0_2018-04-05_16h39_+0200/roddyExecutionStore/exec_180405_163953553_otp-data_WGS/analysisTools/roddyTools/wrapInScript.sh' | |
| jobInfo.startCount == 1 | |
| jobInfo.account == null | |
| jobInfo.server == "pbsserver" | |
| jobInfo.umask == null | |
| jobInfo.parameters == null | |
| jobInfo.parentJobIDs == ["6059780", "6059781"] | |
| jobInfo.otherSettings == null | |
| jobInfo.jobState == JobState.COMPLETED_UNKNOWN | |
| jobInfo.userTime == null | |
| jobInfo.systemTime == null | |
| jobInfo.pendReason == null | |
| jobInfo.execHome == null | |
| jobInfo.execUserName == null | |
| jobInfo.pidStr == null | |
| jobInfo.pgidStr == null | |
| jobInfo.exitCode == 0 | |
| jobInfo.jobGroup == null | |
| jobInfo.description == null | |
| jobInfo.execCwd == null | |
| jobInfo.askedHostsStr == null | |
| jobInfo.cwd == null | |
| jobInfo.projectName == null | |
| jobInfo.cpuTime == Duration.ZERO | |
| jobInfo.runTime == Duration.ofSeconds(10) | |
| jobInfo.timeUserSuspState == null | |
| jobInfo.timePendState == null | |
| jobInfo.timePendSuspState == null | |
| jobInfo.timeSystemSuspState == null | |
| jobInfo.timeUnknownState == null | |
| jobInfo.timeOfCalculation == null | |
| } | |
| void "processQstatOutput, qstat with empty XML output"() { | |
| jobInfo == expected | |
| } | |
| void "test queryExtendedJobInfoByID with empty XML output"() { | |
| given: | |
| String rawXMLOutput =''' | |
| <Data> | |
| <Job> | |
| <Job_Id>15020227.testServer</Job_Id> | |
| </Job> | |
| </Data> | |
| ''' | |
| def jm = createJobManagerWithModifiedExecutionService("queryExtendedJobInfoWithEmptyXML.xml") | |
| when: | |
| Map<BEJobID, GenericJobInfo> jobInfo = manager.processQstatOutputFromXML(rawXMLOutput) | |
| Map<BEJobID, ExtendedJobInfo> result = jm.queryExtendedJobInfo([testJobID]) | |
| def jobInfo = result.get(testJobID) | |
| then: | |
| jobInfo.size() == 1 | |
| result.size() == 1 | |
| jobInfo | |
| } | |
| void "processQstatOutput, qstat with XML output"() { | |
| void "test queryExtendedJobInfoByID, replace placeholder PBS_JOBID in logFile and errorLogFile with job id "() { | |
| given: | |
| def jm = createJobManagerWithModifiedExecutionService("queryExtendedJobInfoWithPlaceholderReplacement.xml") | |
| when: | |
| Map<BEJobID, GenericJobInfo> jobInfo = manager.processQstatOutputFromXML(rawXmlExample) | |
| Map<BEJobID, ExtendedJobInfo> result = jm.queryExtendedJobInfo([testJobID]) | |
| def jobInfo = result.get(testJobID) | |
| then: | |
| jobInfo.size() == 1 | |
| jobInfo.get(new BEJobID("15020227")).jobName == "workflow_test" | |
| result.size() == 1 | |
| jobInfo.logFile.absolutePath == "/logging_root_path/logfile.o22005.testServer" | |
| jobInfo.errorLogFile.absolutePath == "/logging_root_path/logfile.e22005.testServer" | |
| } | |
| def "test getEnvironmentVariableGlobs"() { | |
| return null | |
| } | |
| void "processQstatOutput, replace placeholder PBS_JOBID in logFile and errorLogFile with job id "() { | |
| given: | |
| String rawXMLOutput=''' | |
| <Data> | |
| <Job> | |
| <Job_Id>15976927.testServer</Job_Id> | |
| <Output_Path>/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o$PBS_JOBID</Output_Path> | |
| <Error_Path>tbi-pbs:/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e$PBS_JOBID</Error_Path> | |
| </Job> | |
| </Data> | |
| ''' | |
| def "test getDefaultForHoldJobsEnabled"() { | |
| return null | |
| } | |
| when: | |
| Map<BEJobID, GenericJobInfo> jobInfo = manager.processQstatOutputFromXML(rawXMLOutput) | |
| def "test isHoldJobsEnabled"() { | |
| return null | |
| } | |
| then: | |
| jobInfo.size() == 1 | |
| jobInfo.get(new BEJobID("15976927")).logFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o15976927.testServer" | |
| jobInfo.get(new BEJobID("15976927")).errorLogFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e15976927.testServer" | |
| def "test getUserEmail"() { | |
| return null | |
| } | |
| def "test getUserMask"() { | |
| return null | |
| } | |
| void "parseColonSeparatedHHMMSSDuration, parse duration"() { | |
| def "test getUserGroup"() { | |
| return null | |
| } | |
| given: | |
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | |
| method.setAccessible(true) | |
| def "test getUserAccount"() { | |
| return null | |
| } | |
| expect: | |
| parsedDuration == method.invoke(null, input) | |
| def "test executesWithoutJobSystem"() { | |
| return null | |
| } | |
| where: | |
| input || parsedDuration | |
| "00:00:00" || Duration.ofSeconds(0) | |
| "24:00:00" || Duration.ofHours(24) | |
| "119:00:00" || Duration.ofHours(119) | |
| def "test convertResourceSet"() { | |
| return null | |
| } | |
| void "parseColonSeparatedHHMMSSDuration, parse duration fails"() { | |
| def "test collectJobIDsFromJobs"() { | |
| return null | |
| } | |
| given: | |
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | |
| method.setAccessible(true) | |
| def "test extractAndSetJobResultFromExecutionResult"() { | |
| return null | |
| } | |
| when: | |
| method.invoke(null, "02:42") | |
| def "test createCommand"() { | |
| return null | |
| } | |
| then: | |
| InvocationTargetException e = thrown(InvocationTargetException) | |
| e.targetException.message == "Duration string is not of the format HH+:MM:SS: '02:42'" | |
| def "test parseJobID"() { | |
| return null | |
| } | |
| def "test parseJobState"() { | |
| return null | |
| } | |
| def "test executeStartHeldJobs"() { | |
| return null | |
| } | |
| def "test executeKillJobs"() { | |
| return null | |
| } | |
| def "test submitJob"() { | |
| // Implement test logic here | |
| } | |
| def "test addToListOfStartedJobs"() { | |
| return null | |
| } | |
| def "test startHeldJobs"() { | |
| return null | |
| } | |
| def "test killJobs"() { | |
| return null | |
| } | |
| def "test queryJobStateByJob"() { | |
| return null | |
| } | |
| def "test queryJobStateByID"() { | |
| return null | |
| } | |
| def "test queryJobInfByJob"() { | |
| return null | |
| } | |
| def "test queryJobInfByID"() { | |
| return null | |
| } | |
| def "test queryAllJobInf"() { | |
| return null | |
| } | |
| def "test queryExtendedJobInfByJob"() { | |
| return null | |
| } | |
| def "test queryExtendedJobInfByID"() { | |
| return null | |
| } | |
| void "test queryExtendedJobInfByID with queued job"() { | |
| given: | |
| def jobManager = createJobManagerWithModifiedExecutionService("processExtendedQstatXMLOutputWithQueuedJob.xml") | |
| ExtendedJobInfo expected = getGenericJobInfoObjectForTests() | |
| when: | |
| Map<BEJobID, ExtendedJobInfo> result = jobManager.queryExtendedJobInfo([testJobID]) | |
| ExtendedJobInfo jobInfo = result.get(testJobID) | |
| then: | |
| result.size() == 1 | |
| jobInfo.requestedResources == expected.requestedResources | |
| jobInfo.usedResources == expected.usedResources | |
| jobInfo.submitTime == expected.submitTime | |
| jobInfo == expected | |
| } | |
| void "test queryExtendedJobInfByID with finished job"() { | |
| given: | |
| def jobManager = createJobManagerWithModifiedExecutionService("processExtendedQstatXMLOutputWithFinishedJob.xml") | |
| ExtendedJobInfo expected = getGenericJobInfoObjectForTests() | |
| expected.usedResources = new ResourceSet(null, new BufferValue(6064, BufferUnit.k), null, null, Duration.ofSeconds(6), null, "otp", null) | |
| expected.startTime = ZonedDateTime.of(2018, 4, 5, 16, 39, 43, 0, ZoneId.systemDefault()) | |
| expected.endTime = ZonedDateTime.of(2018, 4, 5, 16, 39, 54, 0, ZoneId.systemDefault()) | |
| expected.executionHosts = ["exec-host"] | |
| expected.startCount = 1 | |
| expected.parentJobIDs = ["6059780", "6059781"] | |
| expected.jobState = JobState.COMPLETED_UNKNOWN | |
| expected.exitCode = 0 | |
| expected.cpuTime = Duration.ZERO | |
| expected.runTime = Duration.ofSeconds(10) | |
| when: | |
| Map<BEJobID, ExtendedJobInfo> result = jobManager.queryExtendedJobInfo([testJobID]) | |
| ExtendedJobInfo jobInfo = result.get(testJobID) | |
| then: | |
| result.size() == 1 | |
| jobInfo == expected | |
| } | |
| void "test queryExtendedJobInfoByID with empty XML output"() { | |
| given: | |
| def jm = createJobManagerWithModifiedExecutionService("queryExtendedJobInfoWithEmptyXML.xml") | |
| when: | |
| Map<BEJobID, ExtendedJobInfo> result = jm.queryExtendedJobInfo([testJobID]) | |
| def jobInfo = result.get(testJobID) | |
| then: | |
| result.size() == 1 | |
| jobInfo | |
| } | |
| void "test queryExtendedJobInfoByID, replace placeholder PBS_JOBID in logFile and errorLogFile with job id "() { | |
| given: | |
| def jm = createJobManagerWithModifiedExecutionService("queryExtendedJobInfoWithPlaceholderReplacement.xml") | |
| when: | |
| Map<BEJobID, ExtendedJobInfo> result = jm.queryExtendedJobInfo([testJobID]) | |
| def jobInfo = result.get(testJobID) | |
| then: | |
| result.size() == 1 | |
| jobInfo.logFile.absolutePath == "/logging_root_path/logfile.o22005.testServer" | |
| jobInfo.errorLogFile.absolutePath == "/logging_root_path/logfile.e22005.testServer" | |
| } | |
| def "test getEnvironmentVariableGlobs"() { | |
| return null | |
| } | |
| def "test getDefaultForHoldJobsEnabled"() { | |
| return null | |
| } | |
| def "test isHoldJobsEnabled"() { | |
| return null | |
| } | |
| def "test getUserEmail"() { | |
| return null | |
| } | |
| def "test getUserMask"() { | |
| return null | |
| } | |
| def "test getUserGroup"() { | |
| return null | |
| } | |
| def "test getUserAccount"() { | |
| return null | |
| } | |
| def "test executesWithoutJobSystem"() { | |
| return null | |
| } | |
| def "test convertResourceSet"() { | |
| return null | |
| } | |
| def "test collectJobIDsFromJobs"() { | |
| return null | |
| } | |
| def "test extractAndSetJobResultFromExecutionResult"() { | |
| return null | |
| } | |
| def "test createCommand"() { | |
| return null | |
| } | |
| def "test parseJobID"() { | |
| return null | |
| } | |
| def "test parseJob | |
| </details> | |
| <!-- suggestion_end --> | |
| <!-- This is an auto-generated comment by CodeRabbit --> |
| def "test queryJobStateByJob"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryJobStateByID"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryJobStatesByJob"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryJobStatesByID"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryAllJobStates"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryJobStates with overdue finish date"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryExtendedJobStatesByJob"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryExtendedJobStatesById"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test queryExtendedJobStatesById with overdue date"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getEnvironmentVariableGlobs"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getDefaultForHoldJobsEnabled"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test isHoldJobsEnabled"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getUserEmail"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getUserMask"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getUserGroup"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getUserAccount"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test executesWithoutJobSystem"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test convertResourceSet"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test collectJobIDsFromJobs"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test extractAndSetJobResultFromExecutionResult"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test createCommand"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test parseJobID"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test parseJobState"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test executeStartHeldJobs"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test executeKillJobs"() { | ||
| return null | ||
| } | ||
| } |
There was a problem hiding this comment.
The test cases are currently empty and return null. It's important to implement these tests to ensure the functionality of the methods in the PBSJobManager class.
- return null
+ // TODO: Implement this testCommitable suggestion (Beta)
| package de.dkfz.roddy.execution.jobs.cluster | |
| import de.dkfz.roddy.TestExecutionService | |
| import de.dkfz.roddy.execution.jobs.BatchEuphoriaJobManager | |
| import de.dkfz.roddy.execution.jobs.cluster.pbs.PBSJobManager | |
| /** | |
| * Test spec for JobManagerImplementationBaseSpecSpec | |
| */ | |
| class JobManagerImplementationBaseSpecSpec extends JobManagerImplementationBaseSpec<PBSJobManager> { | |
| def "test createJobManagerWithModifiedExecutionService"() { | |
| when: | |
| BatchEuphoriaJobManager jm = createJobManagerWithModifiedExecutionService("createJobManagerWithModifiedExecutionServiceTest.txt") | |
| def expected = ["Line1", "Line2", "Line3"] | |
| def res0 = jm.executionService.execute("blabla").resultLines | |
| def res1 = jm.executionService.execute("something else").resultLines | |
| def res2 = jm.executionService.execute("just a test").resultLines | |
| then: | |
| res0 == expected | |
| res1 == expected | |
| res2 == expected | |
| } | |
| def "test createJobManager"() { | |
| when: | |
| BatchEuphoriaJobManager jm = createJobManager() | |
| then: | |
| jm instanceof PBSJobManager | |
| jm.getExecutionService() instanceof TestExecutionService | |
| } | |
| def "test submitJob"() { | |
| return null | |
| } | |
| def "test addToListOfStartedJobs"() { | |
| return null | |
| } | |
| def "test startHeldJobs"() { | |
| return null | |
| } | |
| def "test killJobs"() { | |
| return null | |
| } | |
| def "test queryJobStateByJob"() { | |
| return null | |
| } | |
| def "test queryJobStateByID"() { | |
| return null | |
| } | |
| def "test queryJobStatesByJob"() { | |
| return null | |
| } | |
| def "test queryJobStatesByID"() { | |
| return null | |
| } | |
| def "test queryAllJobStates"() { | |
| return null | |
| } | |
| def "test queryJobStates with overdue finish date"() { | |
| return null | |
| } | |
| def "test queryExtendedJobStatesByJob"() { | |
| return null | |
| } | |
| def "test queryExtendedJobStatesById"() { | |
| return null | |
| } | |
| def "test queryExtendedJobStatesById with overdue date"() { | |
| return null | |
| } | |
| def "test getEnvironmentVariableGlobs"() { | |
| return null | |
| } | |
| def "test getDefaultForHoldJobsEnabled"() { | |
| return null | |
| } | |
| def "test isHoldJobsEnabled"() { | |
| return null | |
| } | |
| def "test getUserEmail"() { | |
| return null | |
| } | |
| def "test getUserMask"() { | |
| return null | |
| } | |
| def "test getUserGroup"() { | |
| return null | |
| } | |
| def "test getUserAccount"() { | |
| return null | |
| } | |
| def "test executesWithoutJobSystem"() { | |
| return null | |
| } | |
| def "test convertResourceSet"() { | |
| return null | |
| } | |
| def "test collectJobIDsFromJobs"() { | |
| return null | |
| } | |
| def "test extractAndSetJobResultFromExecutionResult"() { | |
| return null | |
| } | |
| def "test createCommand"() { | |
| return null | |
| } | |
| def "test parseJobID"() { | |
| return null | |
| } | |
| def "test parseJobState"() { | |
| return null | |
| } | |
| def "test executeStartHeldJobs"() { | |
| return null | |
| } | |
| def "test executeKillJobs"() { | |
| return null | |
| } | |
| } | |
| package de.dkfz.roddy.execution.jobs.cluster | |
| import de.dkfz.roddy.TestExecutionService | |
| import de.dkfz.roddy.execution.jobs.BatchEuphoriaJobManager | |
| import de.dkfz.roddy.execution.jobs.cluster.pbs.PBSJobManager | |
| /** | |
| * Test spec for JobManagerImplementationBaseSpecSpec | |
| */ | |
| class JobManagerImplementationBaseSpecSpec extends JobManagerImplementationBaseSpec<PBSJobManager> { | |
| def "test createJobManagerWithModifiedExecutionService"() { | |
| when: | |
| BatchEuphoriaJobManager jm = createJobManagerWithModifiedExecutionService("createJobManagerWithModifiedExecutionServiceTest.txt") | |
| def expected = ["Line1", "Line2", "Line3"] | |
| def res0 = jm.executionService.execute("blabla").resultLines | |
| def res1 = jm.executionService.execute("something else").resultLines | |
| def res2 = jm.executionService.execute("just a test").resultLines | |
| then: | |
| res0 == expected | |
| res1 == expected | |
| res2 == expected | |
| } | |
| def "test createJobManager"() { | |
| when: | |
| BatchEuphoriaJobManager jm = createJobManager() | |
| then: | |
| jm instanceof PBSJobManager | |
| jm.getExecutionService() instanceof TestExecutionService | |
| } | |
| def "test submitJob"() { | |
| // TODO: Implement this test | |
| } | |
| def "test addToListOfStartedJobs"() { | |
| // TODO: Implement this test | |
| } | |
| def "test startHeldJobs"() { | |
| // TODO: Implement this test | |
| } | |
| def "test killJobs"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryJobStateByJob"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryJobStateByID"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryJobStatesByJob"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryJobStatesByID"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryAllJobStates"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryJobStates with overdue finish date"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryExtendedJobStatesByJob"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryExtendedJobStatesById"() { | |
| // TODO: Implement this test | |
| } | |
| def "test queryExtendedJobStatesById with overdue date"() { | |
| // TODO: Implement this test | |
| } | |
| def "test getEnvironmentVariableGlobs"() { | |
| // TODO: Implement this test | |
| } | |
| def "test getDefaultForHoldJobsEnabled"() { | |
| // TODO: Implement this test | |
| } | |
| def "test isHoldJobsEnabled"() { | |
| // TODO: Implement this test | |
| } | |
| def "test getUserEmail"() { | |
| // TODO: Implement this test | |
| } | |
| def "test getUserMask"() { | |
| // TODO: Implement this test | |
| } | |
| def "test getUserGroup"() { | |
| // TODO: Implement this test | |
| } | |
| def "test getUserAccount"() { | |
| // TODO: Implement this test | |
| } | |
| def "test executesWithoutJobSystem"() { | |
| // TODO: Implement this test | |
| } | |
| def "test convertResourceSet"() { | |
| // TODO: Implement this test | |
| } | |
| def "test collectJobIDsFromJobs"() { | |
| // TODO: Implement this test | |
| } | |
| def "test extractAndSetJobResultFromExecutionResult"() { | |
| // TODO: Implement this test | |
| } | |
| def "test createCommand"() { | |
| // TODO: Implement this test | |
| } | |
| def "test parseJobID"() { | |
| // TODO: Implement this test | |
| } | |
| def "test parseJobState"() { | |
| // TODO: Implement this test | |
| } | |
| def "test executeStartHeldJobs"() { | |
| // TODO: Implement this test | |
| } | |
| def "test executeKillJobs"() { | |
| // TODO: Implement this test | |
| } | |
| } |
| return null | ||
| } | ||
|
|
||
| void "processQstatOutput, replace placeholder PBS_JOBID in logFile and errorLogFile with job id "() { | ||
| given: | ||
| String rawXMLOutput=''' | ||
| <Data> | ||
| <Job> | ||
| <Job_Id>15976927.testServer</Job_Id> | ||
| <Output_Path>/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o$PBS_JOBID</Output_Path> | ||
| <Error_Path>tbi-pbs:/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e$PBS_JOBID</Error_Path> | ||
| </Job> | ||
| </Data> | ||
| ''' | ||
| def "test getDefaultForHoldJobsEnabled"() { | ||
| return null | ||
| } | ||
|
|
||
| when: | ||
| Map<BEJobID, GenericJobInfo> jobInfo = manager.processQstatOutputFromXML(rawXMLOutput) | ||
| def "test isHoldJobsEnabled"() { | ||
| return null | ||
| } | ||
|
|
||
| then: | ||
| jobInfo.size() == 1 | ||
| jobInfo.get(new BEJobID("15976927")).logFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o15976927.testServer" | ||
| jobInfo.get(new BEJobID("15976927")).errorLogFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e15976927.testServer" | ||
| def "test getUserEmail"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test getUserMask"() { | ||
| return null | ||
| } | ||
|
|
||
| void "parseColonSeparatedHHMMSSDuration, parse duration"() { | ||
| def "test getUserGroup"() { | ||
| return null | ||
| } | ||
|
|
||
| given: | ||
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | ||
| method.setAccessible(true) | ||
| def "test getUserAccount"() { | ||
| return null | ||
| } | ||
|
|
||
| expect: | ||
| parsedDuration == method.invoke(null, input) | ||
| def "test executesWithoutJobSystem"() { | ||
| return null | ||
| } | ||
|
|
||
| where: | ||
| input || parsedDuration | ||
| "00:00:00" || Duration.ofSeconds(0) | ||
| "24:00:00" || Duration.ofHours(24) | ||
| "119:00:00" || Duration.ofHours(119) | ||
| def "test convertResourceSet"() { | ||
| return null | ||
| } | ||
|
|
||
|
|
||
| void "parseColonSeparatedHHMMSSDuration, parse duration fails"() { | ||
| def "test collectJobIDsFromJobs"() { | ||
| return null | ||
| } | ||
|
|
||
| given: | ||
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | ||
| method.setAccessible(true) | ||
| def "test extractAndSetJobResultFromExecutionResult"() { | ||
| return null | ||
| } | ||
|
|
||
| when: | ||
| method.invoke(null, "02:42") | ||
| def "test createCommand"() { | ||
| return null | ||
| } | ||
|
|
||
| then: | ||
| InvocationTargetException e = thrown(InvocationTargetException) | ||
| e.targetException.message == "Duration string is not of the format HH+:MM:SS: '02:42'" | ||
| def "test parseJobID"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test parseJobState"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test executeStartHeldJobs"() { | ||
| return null | ||
| } | ||
|
|
||
| def "test executeKillJobs"() { | ||
| return null | ||
| } |
There was a problem hiding this comment.
Several new test methods have been added but are not implemented yet. These methods should be implemented to ensure comprehensive testing of the PBSJobManager class.
- def "test getEnvironmentVariableGlobs"() {
- return null
- }
+ def "test getEnvironmentVariableGlobs"() {
+ // Implement test logic here
+ }Commitable suggestion (Beta)
| def "test getEnvironmentVariableGlobs"() { | |
| return null | |
| } | |
| void "processQstatOutput, replace placeholder PBS_JOBID in logFile and errorLogFile with job id "() { | |
| given: | |
| String rawXMLOutput=''' | |
| <Data> | |
| <Job> | |
| <Job_Id>15976927.testServer</Job_Id> | |
| <Output_Path>/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o$PBS_JOBID</Output_Path> | |
| <Error_Path>tbi-pbs:/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e$PBS_JOBID</Error_Path> | |
| </Job> | |
| </Data> | |
| ''' | |
| def "test getDefaultForHoldJobsEnabled"() { | |
| return null | |
| } | |
| when: | |
| Map<BEJobID, GenericJobInfo> jobInfo = manager.processQstatOutputFromXML(rawXMLOutput) | |
| def "test isHoldJobsEnabled"() { | |
| return null | |
| } | |
| then: | |
| jobInfo.size() == 1 | |
| jobInfo.get(new BEJobID("15976927")).logFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.o15976927.testServer" | |
| jobInfo.get(new BEJobID("15976927")).errorLogFile.toString() == "/logging_root_path/clusterLog/2017-07-07/workflow_test/snvFilter.e15976927.testServer" | |
| def "test getUserEmail"() { | |
| return null | |
| } | |
| def "test getUserMask"() { | |
| return null | |
| } | |
| void "parseColonSeparatedHHMMSSDuration, parse duration"() { | |
| def "test getUserGroup"() { | |
| return null | |
| } | |
| given: | |
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | |
| method.setAccessible(true) | |
| def "test getUserAccount"() { | |
| return null | |
| } | |
| expect: | |
| parsedDuration == method.invoke(null, input) | |
| def "test executesWithoutJobSystem"() { | |
| return null | |
| } | |
| where: | |
| input || parsedDuration | |
| "00:00:00" || Duration.ofSeconds(0) | |
| "24:00:00" || Duration.ofHours(24) | |
| "119:00:00" || Duration.ofHours(119) | |
| def "test convertResourceSet"() { | |
| return null | |
| } | |
| void "parseColonSeparatedHHMMSSDuration, parse duration fails"() { | |
| def "test collectJobIDsFromJobs"() { | |
| return null | |
| } | |
| given: | |
| Method method = ClusterJobManager.class.getDeclaredMethod("parseColonSeparatedHHMMSSDuration", String) | |
| method.setAccessible(true) | |
| def "test extractAndSetJobResultFromExecutionResult"() { | |
| return null | |
| } | |
| when: | |
| method.invoke(null, "02:42") | |
| def "test createCommand"() { | |
| return null | |
| } | |
| then: | |
| InvocationTargetException e = thrown(InvocationTargetException) | |
| e.targetException.message == "Duration string is not of the format HH+:MM:SS: '02:42'" | |
| def "test parseJobID"() { | |
| return null | |
| } | |
| def "test parseJobState"() { | |
| return null | |
| } | |
| def "test executeStartHeldJobs"() { | |
| return null | |
| } | |
| def "test executeKillJobs"() { | |
| return null | |
| } | |
| def "test getEnvironmentVariableGlobs"() { | |
| // Implement test logic here | |
| } | |
| def "test getDefaultForHoldJobsEnabled"() { | |
| return null | |
| } | |
| def "test isHoldJobsEnabled"() { | |
| return null | |
| } | |
| def "test getUserEmail"() { | |
| return null | |
| } | |
| def "test getUserMask"() { | |
| return null | |
| } | |
| def "test getUserGroup"() { | |
| return null | |
| } | |
| def "test getUserAccount"() { | |
| return null | |
| } | |
| def "test executesWithoutJobSystem"() { | |
| return null | |
| } | |
| def "test convertResourceSet"() { | |
| return null | |
| } | |
| def "test collectJobIDsFromJobs"() { | |
| return null | |
| } | |
| def "test extractAndSetJobResultFromExecutionResult"() { | |
| return null | |
| } | |
| def "test createCommand"() { | |
| return null | |
| } | |
| def "test parseJobID"() { | |
| return null | |
| } | |
| def "test parseJobState"() { | |
| return null | |
| } | |
| def "test executeStartHeldJobs"() { | |
| return null | |
| } | |
| def "test executeKillJobs"() { | |
| return null | |
| } |
| String getQueryCommandForJobInfo() { | ||
| return null | ||
| } | ||
|
|
||
| @Override | ||
| String getExtendedQueryJobStatesCommand() { | ||
| String getQueryCommandForExtendedJobInfo() { | ||
| return null | ||
| } |
There was a problem hiding this comment.
The getQueryCommandForJobInfo and getQueryCommandForExtendedJobInfo methods have been added but return null. If these methods are intended to be overridden in subclasses, consider making them abstract. If they are not intended to be overridden, they should return a valid command string.
- String getQueryCommandForJobInfo() {
- return null
- }
- String getQueryCommandForExtendedJobInfo() {
- return null
- }
+ abstract String getQueryCommandForJobInfo();
+ abstract String getQueryCommandForExtendedJobInfo();Commitable suggestion (Beta)
| String getQueryCommandForJobInfo() { | |
| return null | |
| } | |
| @Override | |
| String getExtendedQueryJobStatesCommand() { | |
| String getQueryCommandForExtendedJobInfo() { | |
| return null | |
| } | |
| abstract String getQueryCommandForJobInfo(); | |
| abstract String getQueryCommandForExtendedJobInfo(); |
No description provided.