novelrt / NovelRT

A cross-platform 2D game engine accompanied by a strong toolset for visual novels.
MIT License
183 stars 43 forks source link

Added options to the test target #511

Closed bjauny closed 1 year ago

bjauny commented 1 year ago

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) This adds options used in the Interop project to the Engine_Tests project.

Is there an open issue that this resolves? If so, please link it here. Fixes #506

What is the current behavior? (You can also link to an open issue here) There is no specific options in the Engine_Tests project, making it not inline with the rest of the projects.

What is the new behavior (if this is a feature change)? Engine_Tests now uses the same options as the other ones.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) It sould be transparent.

Other information:

RubyNova commented 1 year ago

Oh holy hell that was fast I just got back to my hotel! Didn't even have time to assign you the second issue hahahahaha

Give me 5 I'll review it I think Monica is away right now

bjauny commented 1 year ago

I fixed the MSVC errors and squashed my commits in one.

RubyNova commented 1 year ago

There was no need to close this - can we reopen it?

bjauny commented 1 year ago

There was no need to close this - can we reopen it?

Yeah, not sure what happened here, I'm still on it.

bjauny commented 1 year ago

Sorry for all the requests I thought I could add both of you.

RubyNova commented 1 year ago

You're fine, don't worry about it - I'm just happy to be getting outside help, as in non-maintainers, lol

bjauny commented 1 year ago

Looks good at a quick skim through. @RubyNova are the test changes good too?

Thanks!

bjauny commented 1 year ago

All change requests have been done in the last commit.

I'm not very satisfied with the handling of error messages in the fixed TODOs though. The strings are not public so the tests need to define the exact string to be compared with.

Maybe a future issue would be to make this messages as const public values, at least for tests sake?

RubyNova commented 1 year ago

We should have functions that return/set the correct string for getLastError in that very same header you included in the tests. Since it's a C header it should just be working, but if it's not could you clarify why? Finding this stuff on mobile in the middle of a hotel is hard hahahaha

bjauny commented 1 year ago

We should have functions that return/set the correct string for getLastError in that very same header you included in the tests. Since it's a C header it should just be working, but if it's not could you clarify why? Finding this stuff on mobile in the middle of a hotel is hard hahahaha

Actually, I only have access to functions that set the error message, and one generic getter, that returns the actual char *. The thing is, the actual values of the messages are in NrtErrorHandling.cpp and I can't use errMsgIsNullInstanceProvided outside of this file for example.

RubyNova commented 1 year ago

You can use that function I believe - maybe it's not included in the header you are using though. It's been a while since I structured this. You should be able to use your IDE to jump to the declaration of that function.

RubyNova commented 1 year ago

Just to add to my approval here - it seems this PR is currently failing clang format. Our CI spits out a git patch file you can apply that will make it pass.

Just download that, apply, make sure stuff still works, and then push up and all should be good. :)

bjauny commented 1 year ago

@RubyNova The clang step failed for a reason that I can't understand. Is there something I sould change on my side? Or should it be run again?

RubyNova commented 1 year ago

See previous comment - but basically we have a clang format file that enforces our style rules.

Just do the steps outlined above and as long as it's all working you can just push up.

If stuff breaks and you can't figure out why, do just give us a shout. :D