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.

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

Add test to prevent regressions if BadLayerHandler does not work during packaging or process_projectfile #1089

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

gounux
Copy link
Contributor

@gounux gounux commented Dec 20, 2024

This PR will :

  • Create a simple_bumblebees_wrong_localized.qgs QGIS project file with 3 layers :
    • 1 (invalid) layer with a datasource with non existing filename
    • 1 (invalid) localized layer with a datasource with non existing filename
    • 1 (valid) localized layer with a datasource with existing filename (QFC should consider it as invalid because it is localized)
  • Test that a PROCESS_PROJECTFILE type job parses the layer data correctly
  • Test that a PACKAGE type job parses the layer data correctly

@duke-nyuki
Copy link
Collaborator

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

@gounux in case you did not use already, please install https://plugins.qgis.org/plugins/trackable_project_files/ to make the .qgs file git trackable.

@gounux gounux changed the title [WIP] Add test to prevent regressions if BadLayerHandler does not work during packaging or process_projectfile Add test to prevent regressions if BadLayerHandler does not work during packaging or process_projectfile Dec 21, 2024
@gounux gounux marked this pull request as ready for review December 21, 2024 16:39
Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

A few comments on the test.

@gounux gounux requested a review from suricactus January 4, 2025 11:11
Comment on lines 104 to 108
self.assertFalse(invalid_localized_layer["is_valid"])
self.assertTrue(invalid_localized_layer["is_localized"])
self.assertEquals(
invalid_localized_layer["error_code"], "localized_dataprovider"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you define:

    def assertLayerData(self, layer_data: dict, is_valid: bool, is_localized: bool, error_code: str):
        self.assertEquals(layer_data["is_valid"], is_valid)
        self.assertEquals(layer_data["is_localized"], is_localized)
        self.assertEquals(layer_data["error_code"], error_code)

You can simplify the code and improve readability to something like:

Suggested change
self.assertFalse(invalid_localized_layer["is_valid"])
self.assertTrue(invalid_localized_layer["is_localized"])
self.assertEquals(
invalid_localized_layer["error_code"], "localized_dataprovider"
)
self.assertLayerData(
processfile_layers["invalid_localized_polygon_layer_id"],
False,
True,
"localized_dataprovider",
)

Copy link
Contributor Author

@gounux gounux Jan 7, 2025

Choose a reason for hiding this comment

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

Thanks for the suggestion ! Indeed it makes things more simple.

I've actually been using parameters keywords when calling the function, for more readability, like this :

self.assertLayerData(
  processfile_layers["invalid_localized_polygon_layer_id"],
  is_valid=False,
  is_localized=True,
  error_code="localized_dataprovider",
)

What do you think ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants