bug fix for incomplete xml imports #2468
Conversation
| import xml.parsers.expat | ||
|
|
||
|
|
||
| def verify_waypoint_data(xml_content): |
There was a problem hiding this comment.
This could be an ideal use-case for XML Schemas. That would require more than the stdlib xml support though.
There was a problem hiding this comment.
this is currently only a simple solution. A refactoring needs to be done too.
| from mslib.utils.verify_waypoint_data import verify_waypoint_data | ||
|
|
||
|
|
||
| def test_verify_xml_waypoint(): |
There was a problem hiding this comment.
Do we need to test this specifically or can we cover the entirety of that function by only interacting with MSColab's public interface?
There was a problem hiding this comment.
We have more than one interface. I want to be sure that this is covered when a refactoring is done and whatever sends us data.
There was a problem hiding this comment.
We have more than one interface.
And all of them that interact with the real world have to be tested. The important part is that the servers interface to the outside world does indeed do the validation, not that the validation in itself exists and responds as expected.
I just want to emphasize that a test is meant to codify an expected behavior of the code, and the existence of an internal function called verify_xml_waypoint is not a crucial part to ensure for the future, in my opinion.
Ensuring that coverage doesn't decrease through a refactoring could be accomplished by checking that coverage does not decrease, instead.
whatever sends us data
That's the point, this test does not concern itself with if data send from whereever is actually validated.
There was a problem hiding this comment.
I disagree, I first want to know what kind of failures we are aware off. I also want to know that a new function has no failure when it is designed. For me testdriven means to define what the function should solve. The test describes what I had in mind as I wrote the function.
There was a problem hiding this comment.
I am not sure whether this should be discussed on an open channel. However, the XML is never parsed by the server, so the server should be "safe".
The clients are a different matter though. If we think that XML cannot be trusted, simply switching to defusedxml on the client side should solve this...?
There was a problem hiding this comment.
@joernu76 I think you wanted to respond to #2468 (comment). Yes, you are right, we should take that to a closed channel.
There was a problem hiding this comment.
For me testdriven means to define what the function should solve.
I see that differently. Test-driven means writing tests for the correctness of the change that is to be implemented, in this case for the change that the server and client should verify the xml data. That in the end this function implements the change is an irrelevant implementation detail, as far as testing is concerned IMO. It could be a thousand different functions, and only a single test, as long as the test exercises the change they implement from the codes exposed surface. Testing all of the thousand functions would even be detrimental, because it locks in a particular architectural design, with little to no benefit.
Anyway, no matter if this test is present or not, the client-side validation isn't tested for yet. I.e. I can remove the client-side validation without a test failing.
| @@ -179,8 +179,7 @@ def test_delete_operation(self): | |||
| assert len(messages) == 0 | |||
|
|
|||
| def _example_data(self): | |||
| self.content1 = """\ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| self.content1 = """<?xml version="1.0" encoding="utf-8"?> | |||
There was a problem hiding this comment.
this was also giving an xml.parsers.expat.ExpatError
| if not verify_waypoint_data(xml_content): | ||
| show_popup(self.ui, "Import Failed", f"The file - {file_name}, was not imported!", 0) | ||
| return |
There was a problem hiding this comment.
If the intention is to protect against e.g. maliciously crafted XML or the like, then verification must be the first thing that is done, before using the XML for anything else. In this case though, the XML might be used already here:
Line 1971 in ff744f5
There was a problem hiding this comment.
Also, if function is not None, then that supplied function would need to do the verification as well. But this is a theoretical concern, it seems like for XML files it is always None, and it is only used to provide a way to read CSV files instead.
There was a problem hiding this comment.
a users functions has to return name, waypoints e.g. https://github.com/Open-MSS/MSS/blob/develop/mslib/plugins/io/flitestar.py
How a user can test his functions is a different scope.
tests/_test_mscolab/test_server.py
Outdated
| self.userdata = 'UV10@uv10', 'UV10', 'uv10' | ||
| self.sio = self.sockio.test_client(self.app) |
There was a problem hiding this comment.
Please use this as a contextmanager, so it is disconnected properly afterwards.
There was a problem hiding this comment.
Oh, actually, I don't think it is useable as a contextmanager, unlike the flask test_client. Still, the disconnect is missing.
There was a problem hiding this comment.
unfortunately context manager protocol is not implemented into the SocketIOTestClient for flask-socketio, but hey I can just use it there where I need it.
tests/_test_mscolab/test_server.py
Outdated
| @@ -410,5 +443,5 @@ def _save_content(self, operation, userdata=None): | |||
| userdata = self.userdata | |||
| user = get_user(userdata[0]) | |||
| fm = FileManager(self.app.config["MSCOLAB_DATA_DIR"]) | |||
There was a problem hiding this comment.
The SocketsManager, FileManager and ChatManager instances are only really designed to be singleton objects at the moment; creating more instances of them does not reflect reality, so isn't useful for tests, and might even break stuff as it did with multiple SocketsManagers.
| xml_content = ( | ||
| (flight_track_with_waypoints, True), | ||
| (flight_track_empty, False), | ||
| (flight_track_incomplete, False), | ||
| (flight_track_with_typo, False), | ||
| ) |
There was a problem hiding this comment.
The pytest-idiomatic way for this would be to parametrize the test over these cases: https://docs.pytest.org/en/7.1.x/how-to/parametrize.html
There was a problem hiding this comment.
I know, but then the cases outside the function and I don't see that as better readable.
There was a problem hiding this comment.
Yes, they would be outside the function because each case would then actually be a separate test, as far as pytest is concerned. I find it more understandable when it is clear from the failing test itself which case fails, instead of having to look at the test function and figuring out in which loop iteration the assert fails.
There was a problem hiding this comment.
separating the tests by different names I can agree but for xml the fixture gets unreadable.
There was a problem hiding this comment.
I added one of the linebreak patterns too, which fails too.
There was a problem hiding this comment.
next update on the way
There was a problem hiding this comment.
Having it split into individual functions with descriptive names like you did looks good too though, pick what you prefer.
There was a problem hiding this comment.
compared to individual functions in a failure case that would look like
How would I see now that flight_track_with_typo was the reason?
There was a problem hiding this comment.
I think we keep it, it is good to have such an examples with more content.
There was a problem hiding this comment.
Ahh right, it shows the content of the variable as the test name by default. That can be changed with the ids= option to pytest.mark.parametrize.
* removing pinned xmlschema (#2117) * Stop ignoring some test files (#2291) * Reconfigured tutorial videos for RTFD (#2458) * changed to videos/ * use mss style on rtfd * fix urls and text (#2460) * Bump benc-uk/workflow-dispatch from 1.2.3 to 1.2.4 (#2450) Bumps [benc-uk/workflow-dispatch](https://github.com/benc-uk/workflow-dispatch) from 1.2.3 to 1.2.4. - [Release notes](https://github.com/benc-uk/workflow-dispatch/releases) - [Commits](benc-uk/workflow-dispatch@v1.2.3...v1.2.4) --- updated-dependencies: - dependency-name: benc-uk/workflow-dispatch dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Limit video width to the containing block's width (#2463) * pinning of libtiff is not needed but blocks a more recent libxml2 and others (#2469) * fixed the model.Message description and tests now using op_id (#2470) * bug fix for incomplete xml imports (#2468) validate xml content * prepare v9.2.0 (#2481) --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: Matthias Riße <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Purpose of PR?:
Fixes #2456
Does this PR introduce a breaking change?
If the changes in this PR are manually verified, list down the scenarios covered::
Additional information for reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs
Does this PR results in some Documentation changes?
If yes, include the list of Documentation changes
Checklist:
<type>: <subject>