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

TorqueScript unit test #592

Closed
wants to merge 4 commits into
base: development
from

Conversation

Projects
None yet
5 participants
@Areloch
Contributor

Areloch commented Mar 24, 2014

Adds a unit test to the engine that tests some basic functionality of TorqueScript.

Can easily be extended later to test more things, but this acts as a solid starting point.

@Areloch Areloch changed the title from Ts unit test to TorqueScript unit test Mar 24, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Mar 24, 2014

Contributor

I wonder whether these tests should be written in TS rather than in C++. It's cool that we have a C++ test framework set up, but I wonder what it gives us over the alternative.

Contributor

crabmusket commented Mar 24, 2014

I wonder whether these tests should be written in TS rather than in C++. It's cool that we have a C++ test framework set up, but I wonder what it gives us over the alternative.

@lukaspj

This comment has been minimized.

Show comment
Hide comment
@lukaspj

lukaspj Mar 24, 2014

Contributor

@eightyeight I guess it wouldn't be unit-tests then but rather implementation tests.
What I mean is if we do the tests in TS, a bunch of other things could go wrong rather than the method we are testing, it could be script parsing, compilation, dictionary look-ups. These things are fine to test for as well, but should have their own unit-tests and not be covered by these tests.

Thats IMO ofc.

Edit: oh my bad, I thought this called the direct methods of the SimObject and not evaluated statements, guess I skimmed it a little too fast, then yes I agree it could have been written in TorqueScript as well.

Contributor

lukaspj commented Mar 24, 2014

@eightyeight I guess it wouldn't be unit-tests then but rather implementation tests.
What I mean is if we do the tests in TS, a bunch of other things could go wrong rather than the method we are testing, it could be script parsing, compilation, dictionary look-ups. These things are fine to test for as well, but should have their own unit-tests and not be covered by these tests.

Thats IMO ofc.

Edit: oh my bad, I thought this called the direct methods of the SimObject and not evaluated statements, guess I skimmed it a little too fast, then yes I agree it could have been written in TorqueScript as well.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Mar 24, 2014

Contributor

@lukaspj That's a good point, and a good distinction to make. Maybe we should be testing everything we can in TS, and everything we can't (i.e. the SimDictionary) in C++. But yes, tests in TS should probably be classed as integration tests, unless you're testing units that are themselves written in TS.

Contributor

crabmusket commented Mar 24, 2014

@lukaspj That's a good point, and a good distinction to make. Maybe we should be testing everything we can in TS, and everything we can't (i.e. the SimDictionary) in C++. But yes, tests in TS should probably be classed as integration tests, unless you're testing units that are themselves written in TS.

@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Mar 24, 2014

Contributor

It could have been written in TS, but between possible cross-contamination of other issues that can give false-positives, you also need to consider that means we'd have to start including them in the templates somewhere.

That MAY not be too much of an issue when we get modular templates in, you can just drop in test files and whatnot, but currently that means you're looking to bloat the templates with non-game scripts.
Integrating them into the engine bypasses cross contamination issues as well as template bloat.

Just my thoughts on the approach to it.

Contributor

Areloch commented Mar 24, 2014

It could have been written in TS, but between possible cross-contamination of other issues that can give false-positives, you also need to consider that means we'd have to start including them in the templates somewhere.

That MAY not be too much of an issue when we get modular templates in, you can just drop in test files and whatnot, but currently that means you're looking to bloat the templates with non-game scripts.
Integrating them into the engine bypasses cross contamination issues as well as template bloat.

Just my thoughts on the approach to it.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Mar 24, 2014

Contributor

Looking at it in that light, this route involves bloating the engine source/executable itself with non-game-related code. Admittedly, a tiny amount, but the amount will grow. Or does the existing infrastructure ensure that unit test code is removed from release builds?

I thought about the false-positives angle, but I can't actually think of a case where changes could break useful scripts and not break unit test scripts.

Contributor

crabmusket commented Mar 24, 2014

Looking at it in that light, this route involves bloating the engine source/executable itself with non-game-related code. Admittedly, a tiny amount, but the amount will grow. Or does the existing infrastructure ensure that unit test code is removed from release builds?

I thought about the false-positives angle, but I can't actually think of a case where changes could break useful scripts and not break unit test scripts.

@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Mar 25, 2014

Contributor

Hm, you may be right about the whole false-positive thing. I looked at it from a 'test it close to the source' angle to minimize things breaking, but it may not matter over-all.

As for the bloat issue, in my mind, the templates have enough excess crap they don't need as it is, and the impact the unit tests have on the engine size is functionally non-existent unless we had a TON of them. We have to put the tests somewhere, and it impacts things less if it's in the engine.

To wit, the TS unit test is currently constrained to a single file that compiles into the engine and stops impacting the end product. Whereas to get the same result in script currently, the TASman setup is using 2 script files and added script exec dependencies. (Not ragging on it, just using it as the counter-example) They do the same job, but the level of 'bloat' is different - at least to me.

Contributor

Areloch commented Mar 25, 2014

Hm, you may be right about the whole false-positive thing. I looked at it from a 'test it close to the source' angle to minimize things breaking, but it may not matter over-all.

As for the bloat issue, in my mind, the templates have enough excess crap they don't need as it is, and the impact the unit tests have on the engine size is functionally non-existent unless we had a TON of them. We have to put the tests somewhere, and it impacts things less if it's in the engine.

To wit, the TS unit test is currently constrained to a single file that compiles into the engine and stops impacting the end product. Whereas to get the same result in script currently, the TASman setup is using 2 script files and added script exec dependencies. (Not ragging on it, just using it as the counter-example) They do the same job, but the level of 'bloat' is different - at least to me.

@dottools

This comment has been minimized.

Show comment
Hide comment
@dottools

dottools Mar 25, 2014

Unit tests should only ever be compiled and used when they're explicitly enabled no matter what build type is used. Might need to make this a projectGenerator option like we do for all the other optional components. That way when somebody really does want to run some unit tests all they need to do is run the project manager and generate themselves a unitTest variant of their projects.

dottools commented Mar 25, 2014

Unit tests should only ever be compiled and used when they're explicitly enabled no matter what build type is used. Might need to make this a projectGenerator option like we do for all the other optional components. That way when somebody really does want to run some unit tests all they need to do is run the project manager and generate themselves a unitTest variant of their projects.

@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Mar 25, 2014

Contributor

@dottools
Excellent point. Currently, I'm pretty sure that it's just compiled in. Having the unit tests added as a component to the project would be really smart.

Contributor

Areloch commented Mar 25, 2014

@dottools
Excellent point. Currently, I'm pretty sure that it's just compiled in. Having the unit tests added as a component to the project would be really smart.

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Mar 25, 2014

Contributor

That's where I was going with that question, but yes, they should definitely be in their own PG module.

As for script testing in scripts, I'm on the fence. I definitely agree that the current templates should not include TS unit tests in TS a la Tasman, but I didn't really imagine we'd stick unit tests in until we were refactoring the templates anyway.

Maybe this basic level of unit testing the TS interpreter should be done engine-side, and then obviously when we get to writing script functionality then tests will be in TS. It'd be nice to use the same testing framework in TS and in C++, to be able to run all the unit tests as a big bundle. That may be infeasible, though.

Contributor

crabmusket commented Mar 25, 2014

That's where I was going with that question, but yes, they should definitely be in their own PG module.

As for script testing in scripts, I'm on the fence. I definitely agree that the current templates should not include TS unit tests in TS a la Tasman, but I didn't really imagine we'd stick unit tests in until we were refactoring the templates anyway.

Maybe this basic level of unit testing the TS interpreter should be done engine-side, and then obviously when we get to writing script functionality then tests will be in TS. It'd be nice to use the same testing framework in TS and in C++, to be able to run all the unit tests as a big bundle. That may be infeasible, though.

@Areloch

This comment has been minimized.

Show comment
Hide comment
@Areloch

Areloch Mar 25, 2014

Contributor

@eightyeight

Heh, we could always just set up a unit test template module with the unit test project component. That should cover everything ;)

Contributor

Areloch commented Mar 25, 2014

@eightyeight

Heh, we could always just set up a unit test template module with the unit test project component. That should cover everything ;)

@LuisAntonRebollo

This comment has been minimized.

Show comment
Hide comment
@LuisAntonRebollo

LuisAntonRebollo Mar 26, 2014

Contributor

We need a TS unit test. Good work.

@eightyeight, It'd be nice to use the same testing framework in TS and in C++, to be able to run all the unit tests as a big bundle

+1. I think it's better to move the test to c++

Contributor

LuisAntonRebollo commented Mar 26, 2014

We need a TS unit test. Good work.

@eightyeight, It'd be nice to use the same testing framework in TS and in C++, to be able to run all the unit tests as a big bundle

+1. I think it's better to move the test to c++

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Mar 28, 2014

Contributor

@Areloch let's move ahead then with writing unit tests in C++ for the script engine itself. We should probably start a new branch in one of our repositories.

Contributor

crabmusket commented Mar 28, 2014

@Areloch let's move ahead then with writing unit tests in C++ for the script engine itself. We should probably start a new branch in one of our repositories.

@crabmusket crabmusket added this to the 3.7 milestone Mar 28, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket May 5, 2014

Contributor

@Areloch I just realised this file is full of tabs :P.

Contributor

crabmusket commented May 5, 2014

@Areloch I just realised this file is full of tabs :P.

@crabmusket crabmusket closed this May 5, 2014

@crabmusket crabmusket reopened this May 5, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket May 5, 2014

Contributor

Whoops, no need to close. You could always rebase this branch to switch to spaces. While you're at it, maybe squash some of those minor commits! In fact this whole PR should probably be one commit.

Contributor

crabmusket commented May 5, 2014

Whoops, no need to close. You could always rebase this branch to switch to spaces. While you're at it, maybe squash some of those minor commits! In fact this whole PR should probably be one commit.

@crabmusket crabmusket modified the milestones: 3.6, 3.7 Jul 11, 2014

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Oct 25, 2014

Contributor

Closing for now as it doesn't merge cleanly. Will probably crib the code, though, for #866.

Contributor

crabmusket commented Oct 25, 2014

Closing for now as it doesn't merge cleanly. Will probably crib the code, though, for #866.

@crabmusket crabmusket closed this Oct 25, 2014

@Areloch Areloch deleted the Areloch:TSUnitTest branch Jul 10, 2017

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