Conversation
tests/execution/test_job.py
Outdated
| class TestJobStatus: | ||
| def test_all_statuses_exist(self): | ||
| expected = {"INIT", "QUEUED", "RUNNING", "CANCELLED", "ERROR", "DONE"} | ||
| actual = {s.name for s in JobStatus} | ||
| assert expected == actual | ||
|
|
||
| def test_terminal_statuses(self): | ||
| """DONE, ERROR, CANCELLED should be considered terminal.""" | ||
| terminal = {JobStatus.DONE, JobStatus.ERROR, JobStatus.CANCELLED} | ||
| non_terminal = {JobStatus.INIT, JobStatus.QUEUED, JobStatus.RUNNING} | ||
| assert terminal.isdisjoint(non_terminal) |
There was a problem hiding this comment.
unsure about the usefulness of these tests
There was a problem hiding this comment.
here we can remove the tests, no need.
tests/execution/test_job.py
Outdated
| job = Job(JobType.SAMPLE, circuit, IBMDevice.AER_SIMULATOR, measure) | ||
| assert job.measure is not None | ||
| # measure should be deep-copied | ||
| assert job.measure is not measure |
tests/execution/test_job.py
Outdated
| circuit = QCircuit(2) | ||
| job = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| job.status = JobStatus.DONE | ||
| # Should not raise — no remote lookup attempted for local device |
There was a problem hiding this comment.
the mentioned feature is not tested IMO
tests/execution/test_job.py
Outdated
| circuit = QCircuit(2) | ||
| job = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| job.status = JobStatus.RUNNING | ||
| assert job._status == JobStatus.RUNNING |
There was a problem hiding this comment.
not a super fan of testing setters and init unless they have complex behavior. It adds testing time, makes the overall architecture harder to change, and doesn't really add useful securities to the testing suite
There was a problem hiding this comment.
Agree here we avoid asserting private/internal fields in tests
tests/execution/test_job.py
Outdated
| def test_terminal_status_no_remote_check(self): | ||
| """Once a job reaches a terminal state, the status property should | ||
| return immediately for any device.""" | ||
| circuit = QCircuit(2) | ||
| job = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| for terminal in [JobStatus.DONE, JobStatus.ERROR, JobStatus.CANCELLED]: | ||
| job.status = terminal | ||
| assert job.status == terminal |
tests/execution/test_job.py
Outdated
| class TestJobEquality: | ||
| def test_equal_jobs(self): | ||
| circuit = QCircuit([H(0), CNOT(0, 1)]) | ||
| job1 = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| job2 = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| assert job1 == job2 | ||
|
|
||
| def test_different_type(self): | ||
| circuit = QCircuit([H(0)]) | ||
| measure = BasisMeasure([0], shots=100) | ||
| job1 = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| job2 = Job(JobType.SAMPLE, circuit, IBMDevice.AER_SIMULATOR, measure) | ||
| assert job1 != job2 | ||
|
|
||
| def test_different_device(self): | ||
| circuit = QCircuit([H(0)]) | ||
| job1 = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| job2 = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR_STATEVECTOR) | ||
| assert job1 != job2 | ||
|
|
||
| def test_different_circuit(self): | ||
| circ1 = QCircuit([H(0)]) | ||
| circ2 = QCircuit([X(0)]) | ||
| job1 = Job(JobType.STATE_VECTOR, circ1, IBMDevice.AER_SIMULATOR) | ||
| job2 = Job(JobType.STATE_VECTOR, circ2, IBMDevice.AER_SIMULATOR) | ||
| assert job1 != job2 | ||
|
|
||
| def test_not_equal_to_non_job(self): | ||
| circuit = QCircuit(2) | ||
| job = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| assert job != "not a job" | ||
| assert job != 42 | ||
| assert job != None |
There was a problem hiding this comment.
maybe we can use a pytest.mark.parametrize for these tests ?
tests/execution/test_job.py
Outdated
| def test_repr_no_measure(self): | ||
| circuit = QCircuit(2) | ||
| job = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| r = repr(job) | ||
| assert "Job(" in r | ||
| assert "STATE_VECTOR" in r | ||
| assert "AER_SIMULATOR" in r | ||
|
|
||
| def test_repr_with_measure(self): | ||
| circuit = QCircuit([H(0)]) | ||
| measure = BasisMeasure([0], shots=100) | ||
| job = Job(JobType.SAMPLE, circuit, IBMDevice.AER_SIMULATOR, measure) | ||
| r = repr(job) | ||
| assert "BasisMeasure" in r | ||
|
|
||
| def test_to_dict_keys(self): | ||
| circuit = QCircuit(2) | ||
| job = Job(JobType.STATE_VECTOR, circuit, IBMDevice.AER_SIMULATOR) | ||
| d = job.to_dict() | ||
| assert "job_type" in d | ||
| assert "circuit" in d | ||
| assert "device" in d | ||
| assert "measure" in d | ||
| assert "id" in d | ||
| assert "status" in d | ||
|
|
There was a problem hiding this comment.
these tests look weird to me, why not testing the whole repr at this point ?
| circuit = QCircuit([Rx(theta, 0)]) | ||
| job = generate_job(circuit, IBMDevice.AER_SIMULATOR, {theta: 1.5}) | ||
| # After substitution the circuit should have no free variables | ||
| assert len(job.circuit.variables()) == 0 |
There was a problem hiding this comment.
In general, I like what you did, but I think the important thing to test in this file is mostly the remote thing, we can merge this once you reply to/take into account the comments, but the biggest issue remains :(
Also, a general question: what are the classes for ? I never used them so I'm curious :)
|
@Henri-ColibrITD Thank you for the thorough review! I addressed most of your concerns in this commit and attempted at the remote tests. As for the test classes, they are mainly for grouping, pytest uses them to namespace the -v output, so instead of seeing: you see: It makes it easier to see which area failed when scanning test output. Totally optional, just a small structural choice to keep things organized as the suite grows |
| def test_construction_with_different_devices(self): | ||
| circuit = QCircuit(2) | ||
| devices = [ | ||
| IBMDevice.AER_SIMULATOR, | ||
| ATOSDevice.MYQLM_PYLINALG, | ||
| AWSDevice.BRAKET_LOCAL_SIMULATOR, | ||
| GOOGLEDevice.CIRQ_LOCAL_SIMULATOR, | ||
| ] | ||
| for device in devices: | ||
| job = Job(JobType.STATE_VECTOR, circuit, device) | ||
| assert job.device == device |
There was a problem hiding this comment.
I would remove this test, just looping through devices to assert job.device is device
| def test_all_types_exist(self): | ||
| expected = {"STATE_VECTOR", "SAMPLE", "OBSERVABLE"} | ||
| actual = {t.name for t in JobType} | ||
| assert expected == actual |
There was a problem hiding this comment.
remove this one, doesn't test behaviour
No description provided.