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

Fix errors when testing Resource #81456

Merged
merged 1 commit into from
Sep 14, 2023
Merged

Conversation

AThousandShips
Copy link
Member

@AThousandShips AThousandShips commented Sep 8, 2023

Using has_meta instead of get_meta where the result is expected to not exist.

The warning and errors when saving and loading needs to be investigated, it appears that the binary case doesn't identify cycles like the text case does, and the text case errors but without indicating that it's a cyclic error.

@AThousandShips AThousandShips added this to the 4.2 milestone Sep 8, 2023
@AThousandShips AThousandShips requested a review from a team as a code owner September 8, 2023 14:24
Comment on lines 126 to 129
ERR_PRINT_OFF
ResourceSaver::save(resource_a, save_path_binary);
ResourceSaver::save(resource_a, save_path_text);
ERR_PRINT_ON
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it's correct to silence those. The error message says "bug?", and that means either the message is wrong (this is a normal, expected failure from misusing the API) or there is a bug to be fixed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Then I'll just fix the has_meta and leave that for further investigation

Copy link
Member Author

Choose a reason for hiding this comment

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

Tried investigating the exact details in the code when I was working on this yesterday, and wasn't sure exactly where to go with it

Now only fixing the get_meta part

The oddity is that the binary case stores the resource reference regardless, as it has parsed it all before storing, but it then gets a warning when reading, can silence that one but I think it might be considered an error that it stores cyclic references

The message in the text case should probably be fixed as well, as it might be missing some details.

Both cases do have seemingly cyclic error detection with dedicated messages but doesn't show up for these

Replaces `get_meta` with `has_meta` for cases where the meta is expected
to be empty.
@AThousandShips AThousandShips changed the title Silence errors when testing Resource Fix errors when testing Resource Sep 9, 2023
@YuriSizov YuriSizov merged commit 721cac4 into godotengine:master Sep 14, 2023
15 checks passed
@AThousandShips AThousandShips deleted the test_fix branch September 14, 2023 13:26
@YuriSizov
Copy link
Contributor

Thanks!

@AThousandShips
Copy link
Member Author

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants