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: [Script] GSAdmin.Send() could generate invalid JSON #11250

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

TrueBrain
Copy link
Member

@TrueBrain TrueBrain commented Aug 31, 2023

Motivation / Problem

Given the following script-snippet:

GSAdmin.Send({ test = GSAdmin });

This would send over the Admin Port: { .

Description

A bit of a silly bug: not checking the return value.

Limitations

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

  • The bug fix is important enough to be backported? (label: 'backport requested')
  • This PR touches english.txt or translations? Check the guidelines
  • This PR affects the save game format? (label 'savegame upgrade')
  • This PR affects the GS/AI API? (label 'needs review: Script API')
    • ai_changelog.hpp, game_changelog.hpp need updating.
    • The compatibility wrappers (compat_*.nut) need updating.
  • This PR affects the NewGRF API? (label 'needs review: NewGRF')

Copy link
Contributor

@rubidium42 rubidium42 left a comment

Choose a reason for hiding this comment

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

This looks quite reasonable to me.

However, for more limited testing I can imagine splitting up ScriptAdmin::Send into two functions. One that does all the error checking and creation of the string/return values, and then Send would call that function, in case there is no error (non-empty std::optional<std::string>), it sends the string over the network. In either case it'll return the return value from the inner function.
This inner function can then be unit tested, however not directly using the Squirrel API. So we need someway for writing a small bit of Squirrel code for tests that generates the appropriate HSQUIRRELVM that can be passed to the object under test. Then you can test the inner function for return value, potentially set error messages and whether the string has been set or not.
The actual sending of the script must then still be tested manually, but if the unit test of the inner test is correct, then sending JSON to the admin port either always works or never works (i.e. you flipped the logic whether to send data on the admin port).

I'm not sure whether it is really worth all that effort though for this function. Unit testing MakeJSON on the other hand seems better, although I could imagine that might be possible from within Squirrel land if we were to have an GS API test like we have for the AI API. Though we need to make sure that the return value for "not valid to send" and "sent correctly to anyone that wanted it" (i.e. might not have been actually sent) are different, as they seem to be the same right now.

src/tests/script_admin_json.cpp Outdated Show resolved Hide resolved
@TrueBrain TrueBrain merged commit 993e7be into OpenTTD:master Aug 31, 2023
20 checks passed
@TrueBrain TrueBrain deleted the script-admin-json-bug branch January 18, 2024 18:46
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.

None yet

3 participants