Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added tests for JobManager #263

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added tests for JobManager #263

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 142 additions & 0 deletions tests/testJobManager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
import unittest
import redis
from mock import MagicMock, patch

from jobManager import JobManager
from jobQueue import JobQueue
from tangoObjects import TangoJob, TangoMachine
from config import Config
from preallocator import *

Comment on lines +1 to +10
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix import statements for better maintainability and reliability.

  1. Replace the wildcard import with explicit imports from preallocator
  2. Add missing time module import which is used in test methods

Apply this diff:

 import unittest
 import redis
+ import time
 from mock import MagicMock, patch

 from jobManager import JobManager
 from jobQueue import JobQueue
 from tangoObjects import TangoJob, TangoMachine
 from config import Config
-from preallocator import *
+from preallocator import Preallocator
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import unittest
import redis
from mock import MagicMock, patch
from jobManager import JobManager
from jobQueue import JobQueue
from tangoObjects import TangoJob, TangoMachine
from config import Config
from preallocator import *
import unittest
import redis
import time
from mock import MagicMock, patch
from jobManager import JobManager
from jobQueue import JobQueue
from tangoObjects import TangoJob, TangoMachine
from config import Config
from preallocator import Preallocator
🧰 Tools
🪛 Ruff

9-9: from preallocator import * used; unable to detect undefined names

(F403)


class TestJobManager(unittest.TestCase):
def createTangoMachine(self, image, vmms, vmObj={"cores": 1, "memory": 512}):
"""createTangoMachine - Creates a tango machine object from image"""
return TangoMachine(
name=image,
vmms=vmms,
image="%s" % (image),
cores=vmObj["cores"],
memory=vmObj["memory"],
disk=None,
network=None,
)

Comment on lines +13 to +24
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Fix mutable default argument in createTangoMachine method.

Using mutable default arguments in Python can lead to unexpected behavior if the default value is modified.

Apply this diff:

-    def createTangoMachine(self, image, vmms, vmObj={"cores": 1, "memory": 512}):
+    def createTangoMachine(self, image, vmms, vmObj=None):
         """createTangoMachine - Creates a tango machine object from image"""
+        if vmObj is None:
+            vmObj = {"cores": 1, "memory": 512}
         return TangoMachine(
             name=image,
             vmms=vmms,
             image="%s" % (image),
             cores=vmObj["cores"],
             memory=vmObj["memory"],
             disk=None,
             network=None,
         )
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def createTangoMachine(self, image, vmms, vmObj={"cores": 1, "memory": 512}):
"""createTangoMachine - Creates a tango machine object from image"""
return TangoMachine(
name=image,
vmms=vmms,
image="%s" % (image),
cores=vmObj["cores"],
memory=vmObj["memory"],
disk=None,
network=None,
)
def createTangoMachine(self, image, vmms, vmObj=None):
"""createTangoMachine - Creates a tango machine object from image"""
if vmObj is None:
vmObj = {"cores": 1, "memory": 512}
return TangoMachine(
name=image,
vmms=vmms,
image="%s" % (image),
cores=vmObj["cores"],
memory=vmObj["memory"],
disk=None,
network=None,
)
🧰 Tools
🪛 Ruff

13-13: Do not use mutable data structures for argument defaults

Replace with None; initialize within function

(B006)

def setUp(self):

if Config.USE_REDIS:
__db = redis.StrictRedis(Config.REDIS_HOSTNAME, Config.REDIS_PORT, db=0)
__db.flushall()

if Config.VMMS_NAME == "ec2SSH":
from vmms.ec2SSH import Ec2SSH

vmms = Ec2SSH()
self.preallocator = Preallocator({"ec2SSH": vmms})

elif Config.VMMS_NAME == "localDocker":
from vmms.localDocker import LocalDocker

vmms = LocalDocker()
self.preallocator = Preallocator({"localDocker": vmms})

elif Config.VMMS_NAME == "distDocker":
from vmms.distDocker import DistDocker

vmms = DistDocker()
self.preallocator = Preallocator({"distDocker": vmms})
else:
vmms = None
self.preallocator = Preallocator({"default": vmms})

self.job1 = TangoJob(
name="sample_job_1",
vm="ilter.img",
outputFile="sample_job_1_output",
input=[],
timeout=30,
notifyURL="notifyMeUrl",
maxOutputFileSize=4096,
)

self.job2 = TangoJob(
name="sample_job_2",
vm="ilter.img",
outputFile="sample_job_2_output",
input=[],
timeout=30,
notifyURL="notifyMeUrl",
maxOutputFileSize=4096,
)

self.jobQueue = JobQueue(self.preallocator)
self.jobQueue.reset()
self.jobManager = JobManager(self.jobQueue)
self.vm = self.createTangoMachine(image="autograding_image", vmms=vmms)

def test_start(self):
self.jobManager.start()
self.assertTrue(self.jobManager.running)

def test__getNextID(self):
init_id = self.jobManager.nextId
for i in range(1, Config.MAX_JOBID + 100):
id = self.jobManager._getNextID()
self.assertEqual(init_id + i - 1, id)
self.jobManager.nextId = init_id

def test_manage_worker_works(self):
def mock_getNextPendingJob():
time.sleep(0.2)
self.job1.setId(1)
return self.job1

def mock_reuseVM(job):
job.assigned = True
job.vm = self.vm
self.preallocator.vmms[job.vm.vmms] = self.jobManager.vmms
return self.vm

def mock_worker_init(job, vmms, jobQueue, preallocator, preVM):
pass

self.jobQueue.assignJob = MagicMock()
with patch(
"jobQueue.JobQueue.getNextPendingJob", side_effect=mock_getNextPendingJob
), patch("jobQueue.JobQueue.reuseVM", side_effect=mock_reuseVM), patch(
"worker.Worker.__init__", side_effect=mock_worker_init
):
self.jobManager.start()
time.sleep(2)
self.jobQueue.assignJob.assert_called()

Comment on lines +88 to +112
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve reliability of worker tests.

The worker tests have several potential issues:

  1. Hard-coded sleep times can make tests flaky
  2. Missing cleanup in tearDown
  3. Limited assertion coverage

Consider these improvements:

  1. Use event-based synchronization instead of sleep
  2. Add proper cleanup
  3. Add more assertions for job state

Example implementation:

def test_manage_worker_works(self):
    job_ready = threading.Event()
    
    def mock_getNextPendingJob():
        job_ready.wait(timeout=1)  # More reliable than sleep
        self.job1.setId(1)
        return self.job1

    # ... rest of the test ...
    
    self.jobManager.start()
    job_ready.set()  # Signal that job is ready
    self.jobQueue.assignJob.assert_called_with(self.job1)
    self.assertTrue(self.job1.assigned)
    self.assertEqual(self.job1.vm, self.vm)

Would you like me to provide a complete implementation with these improvements?

Also applies to: 113-139

🧰 Tools
🪛 Ruff

90-90: time may be undefined, or defined from star imports

(F405)

110-110: time may be undefined, or defined from star imports

(F405)

def test_manage_worker_fails(self):
def mock_getNextPendingJob():
time.sleep(2)
self.job1.setId(1)
return self.job1

def mock_reuseVM(job):
job.assigned = True
job.vm = self.vm
self.preallocator.vmms[job.vm.vmms] = self.jobManager.vmms
return self.vm

def mock_worker_init(job, vmms, jobQueue, preallocator, preVM):
raise RuntimeError("Job failed")

self.jobQueue.assignJob = MagicMock()
self.jobQueue.makeDead = MagicMock()

with patch(
"jobQueue.JobQueue.getNextPendingJob", side_effect=mock_getNextPendingJob
), patch("jobQueue.JobQueue.reuseVM", side_effect=mock_reuseVM), patch(
"worker.Worker.__init__", side_effect=mock_worker_init
):
self.jobManager.start()
time.sleep(4)
self.jobQueue.makeDead.assert_called_once()


if __name__ == "__main__":
unittest.main()
Comment on lines +1 to +142
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add missing test cases for comprehensive coverage.

The current test suite is missing several important test cases:

  1. Error handling for invalid job configurations
  2. Concurrent job processing
  3. Job timeout scenarios
  4. Resource cleanup

Would you like me to help generate additional test cases to improve coverage?

🧰 Tools
🪛 Ruff

9-9: from preallocator import * used; unable to detect undefined names

(F403)

13-13: Do not use mutable data structures for argument defaults

Replace with None; initialize within function

(B006)

35-35: Preallocator may be undefined, or defined from star imports

(F405)

41-41: Preallocator may be undefined, or defined from star imports

(F405)

47-47: Preallocator may be undefined, or defined from star imports

(F405)

50-50: Preallocator may be undefined, or defined from star imports

(F405)

90-90: time may be undefined, or defined from star imports

(F405)

110-110: time may be undefined, or defined from star imports

(F405)

115-115: time may be undefined, or defined from star imports

(F405)

137-137: time may be undefined, or defined from star imports

(F405)