chesko256 / LilacFO4

A Papyrus unit test framework for Fallout 4.
MIT License
13 stars 2 forks source link

The functions "it" and "describe" have an unused boolean parameter #1

Closed Scrivener07 closed 7 years ago

Scrivener07 commented 7 years ago

The it and describe functions have an unused boolean parameter. This parameter is also not reflected in the wiki examples. The functions are documented in papyrus as shown below but it is misleading.

none it(string, bool)

Is there a purpose for these bool parameters? This requires that suite and test functions also return a boolean.

Scrivener07 commented 7 years ago

Okay then, I just figured out some stuff. The big one is that if you define a function that returns anything, Papyrus wont care if there is even a return statement in the function body at all..

I got tripped up on the wiki example because it doesnt compile. The suite and test functions on implemented tests need to define a return type.

At first I tried to remove the parameters but that breaks the syntax of describe() and it(). Thats not okay so I tried something else.

The fact that tests need to return a type of bool may also be misleading. Without reading over the base script it looks as if you should be trying to return success/failure or completion. So I tried changing the parameters on "describe" and "it" to take a var instead of a bool. I thought this way it wouldn't mislead into thinking you need to return a completion state and the wiki example would compile as shown.

My thought was that if a function has no return type defined then it implicitly returns "none", because I can pass a "none" to a var. I was wrong about that, it returns a "void" if no return type is defined.

So back to square one. The Boolean is the best option if a type must be returned because we cannot explicitly return a "none". The suites and tests must return a Boolean value but they do not actually need to have any kind of return statement.

TL:DR Fix the wiki example https://github.com/chesko256/LilacFO4/wiki/4.-Example-Test-Suite

chesko256 commented 7 years ago

Yeah, sorry about this confusion.

This difference, and the need for it, are documented on the wiki here: https://github.com/chesko256/LilacFO4/wiki

I'm not surprised that I missed updating an example. I'll fix it soon.

Did you get your test to compile? Did you have any other issues / questions?

chesko256 commented 7 years ago

I have updated the API documentation and the wiki page you noted. Sorry that this wasn't as straight-forward as it should have been. As you discovered yourself, this was to work around quirks of the Fallout 4 Papyrus compiler and to generate as few runtime errors as possible.

I hope you get your test running; let me know if I can provide any other assistance. I'm just happy someone is using Lilac 👍

Scrivener07 commented 7 years ago

I am very impressed so far, I fell in love even before I worked out all the quirks. Here you can see your Lilac in action GasMask_ContextTest.

My only gripe is that tracing goes to the main papyrus log aka "crap storm". I fixed this by recoding Lilac to use my UserLog class. The Log class is flagged as "DebugOnly" so it is stripped out of release builds. Maybe you could make use of the DebugOnly flag in Lilac instead of using the locking bool "enabled".

Anyway the tests have really helped answer questions about my code and in cases proof that it works.

chesko256 commented 7 years ago

Awesome!