Compare datasets - Integrate Quisby into Pbench Server API #3470
Conversation
There was a problem hiding this comment.
The test_unsuccessful_get_with_incorrect_data() test is not working, even though it is passing; and, it appears to have a bad mock setup, which probably isn't helping. There are a few other gaps in the coverage, one in particular which I would like to see closed is the multiple/non-comparable benchmarks error. And, I would like to see a test for comparing public datasets owned by someone else from a non-authorized user (which I think should work and pass...). And, there is a bug which also shows up in two of the tests.
Beyond those, I have a number of other thoughts, suggestions, and nits.
| if not benchmark_type: | ||
| raise APIAbort( | ||
| HTTPStatus.UNSUPPORTED_MEDIA_TYPE, f"Unsupported Benchmark: {benchmark}" | ||
| ) |
There was a problem hiding this comment.
I think we convinced ourselves that UNSUPPORTED_MEDIA_TYPE is not an appropriate status to use here.
I think we settled on BAD_REQUEST. 🤷 @dbutenhof?
There was a problem hiding this comment.
I dislike using the overly generic BAD_REQUEST intensely, it's just that the more I think about it, the less intensely I dislike it compared to all the other options ... 😦
siddardh-ra
added
the
API
There was a problem hiding this comment.
There are still a couple of places where a mock is returning a mock Tarball class object where I'm pretty sure it should be returning an instance of the class. And, there are a couple of other similar sorts of issues (e.g., functions which should be marked @staticmethod). These should be fixed before we merge this PR.
That said, the test coverage for lib/pbench/server/api/resources/datasets_compare.py is 95%, which is excellent -- it's missing jsut two lines -- consider going for 100% (we just need one more test case...). 🏁
Also, there are a few opportunities for "DRYing" the test code out. There are some code blobs which could be refactored and made common, and, better yet, I think there are some opportunities to collapse whole test functions together using Pytest's "parametrization".
And, there are some smaller items for your consideration.
There was a problem hiding this comment.
Oh, this is so close!
Thanks for doing the parametrization -- that's much nicer, I think (although, we did lose some negative assertions in the process, but I think it's worth it).
There are a few rough edges remaining, which we could ignore, but there are more substantial problems with two of the tests which I'd think should be addressed, and that will give you a chance for some additional polishing.
| def test_unsuccessful_get_with_incorrect_data(self, query_get_as, monkeypatch): | ||
| def mock_extract(_tarball_path: Path, _path: str) -> str: | ||
| raise "IncorrectData" |
There was a problem hiding this comment.
Raising a string as an exception results in a a TypeError exception...so...yeah, this results in an exception, which is what you wanted, but it doesn't produce a plausible behavior.
I don't know what exact behavior we want here, but something more like this would be better:
raise RuntimeError("Testing unexpected exception behavior")| def test_tarball_unpack_exception(self, query_get_as, monkeypatch): | ||
| def mock_extract(_tarball_path: Path, _path: str): | ||
| raise TarballUnpackError |
There was a problem hiding this comment.
This raise is raising an exception class; while this does work, what we want to raise is an instance of an exception class...so,
raise TarballUnpackError(_tarball_path, f"Testing unpack exception for path {_path}")(And, you should presumably remove the leading underscore from the parameter names, now that we're using them.)
| def test_tarball_unpack_exception(self, query_get_as, monkeypatch): | ||
| def mock_extract(_tarball_path: Path, _path: str): | ||
| raise TarballUnpackError | ||
|
|
||
| monkeypatch.setattr(CacheManager, "find_dataset", self.mock_find_dataset) | ||
| monkeypatch.setattr(self.MockTarball, "extract", mock_extract) | ||
| monkeypatch.setattr(Metadata, "getvalue", mock_get_value) | ||
| monkeypatch.setattr( | ||
| QuisbyProcessing, "compare_csv_to_json", mock_compare_csv_to_json | ||
| ) | ||
| query_get_as(["uperf_1", "uperf_2"], "test", HTTPStatus.INTERNAL_SERVER_ERROR) |
There was a problem hiding this comment.
Based on looking at the coverage, I don't think this test is working correctly. I think we're expecting this test to raise an APIInternalError at line 110, but Cobertura claims that that line is never executed. Presumably, the exception is being raised at line 102, instead, but I don't know why. (Is there some reason why user test doesn't have access to datasets uperf_1 and uperf_2 such that the mock_find_dataset() call is raising an exception?)
There was a problem hiding this comment.
Yeah @webbnh, It was due to this raise TarballUnpackError as you mentioned in the earlier comment, it expects the instance of the TarballUnpackError which I missed. After adding this suggestion
raise TarballUnpackError(_tarball_path, f"Testing unpack exception for path {_path}") , It was working as expected :)
Fix and debug Refactoring
|
So, I just checked build #13, and it's still showing two gaps in the coverage. I wouldn't block the merge on this basis, except that I think that we think that the existing tests are supposed to cover those gaps. So, I think we need to at least understand why the tests aren't working as expected. 😞 |
Now assuming Jenkins and GitHub do their jobs, someone can merge soon. Sigh. (And, for the record, we had a 100% successful run with 100% coverage of the new module, but Jenkins glitched and couldn't talk to GitHub so the result never propagated. We're hoping for a reprise... of the success, that is, not of the glitch ...) |
Implementation of API to compare multiple datasets using quisby processing
GET /api/v1/compare?datasets=d1,d2,d3Currently, the pquisby package supports only the uperf benchmark