Skip to content

Conversation

@dlealv
Copy link

@dlealv dlealv commented Sep 15, 2021

I wrote a comment when I added de files to the fork but I am not sure it was visible. I am suggesting the following changes:

  1. New attributes, for tracking testing statistics
  2. New attribute for controlling level of output information: _levelInfo, printing just summary information (0) useful for large testing or test detail info (1), under this scenario the output information will the same as previous version. This is the default value.
  3. New printing methods: printSummary() (for printing testing statistics) and printSubHeader() for adding second level of header info, printHeader() Is more intended for the main testing title, and this one for specific sections.
  4. New resetTestCounters() for resetting the statistical counters.
  5. New assertEquals() It allows to test the function to test is equal to an expected result, in case of failing it shows the discrepancy (no need to provide specific message) in the format: result != expectedResult. It has an optional message argument in case the user wants to specify a different output message.
  6. Correction: the print methods printed information even when enable was false. It doesn’t make sense under this scenario to print information. I changed that.

I added also the corresponding testing template code for testing the new changes. So you can have a better understanding of the changes with one sample.

I didn’t understand your approach for using WeakMap So I used a simple declaration for the new attribute.

Please let me know if you have any comment, if you are agreed I can proceed updating the corresponding documentation

Thanks,

David

@wildhog
Suggested the following changes:
- Added additional attributes to generate statistics and counting tests, failed and passed tests
- Added an attribute to control the level of information to send to the console (`_levelInfo`). It allows to generates only summary information
- Reset testing counters
- `printSummary`: Prints the summary of all previous tests executed so far
- `printSubHeader`: To allow the second level of printing information
- `assertEquals`: To be able to test a function and to compare with the expected result. This function allows to automatically creation message for failing tests comparing the result with the expected result. So it is more friendly because you don´t need to provide `message` input argument (optional)

I modified some existing methods to include the new attributes, such as `assert`, and `printHeader`. For `printHeader` I changed the behavior, so it only prints information when the test is enabled, it doesn't make sense to print information when the test is not enabled.

 If you accept the above changes I can help you to update the documentation.

Please let me know and thanks for this contribution, I tried several options for my case, and it is really an elegant approach and very simple.

I am still learning JS, I didn´t quite understand why you are using Weakmap for `_enable`and `_runningInGas`so I didn´t take that road for creating the new attributes.

I am going to upload to this fork a corresponding `TestingTemplateChanges` file so you can run and see the changes I made.

Please let me know, 

Thanks,

David
Added the file for visualizing and testing the changes added.
@WildH0g
Copy link
Owner

WildH0g commented Sep 17, 2021

Thanks very much, I will have a look shortly. Cheers.

Updated the REAMDE file, based on the minor modification added.
@dlealv
Copy link
Author

dlealv commented Sep 22, 2021

WildHog on my fork I updated the README.md file with the changes I proposed. Thanks

Creating a new version and move it to Example
Testing sample for the new changes in version 0.1.1
Minor changes, spelling, reviewed some sentences.
@WildH0g
Copy link
Owner

WildH0g commented Oct 2, 2021

Hi @dlealv,

Thanks a lot for your work and for your ideas! I really appreciate the time you invested to make this library better.

Please allow me to share some of my thoughts.

What I really liked is your actual ideas:

I really liked the fact that you added statistics of how many tests were run, how many passed, and how many failed. I think that’s great, especially for larger projects.

I also like the idea of the silent mode. I can see a use case for this in a large project when you make some changes and you just want to know that nothing is broken, you just run the tests in silent mode and then, only if you did break something, you can go in and check what exactly.

I also like that idea of displaying the discrepancy between the test result and its expected result when the test fails.

Some of the areas that need improvement are mostly related to the implementation.

Speaking of the WeakMap, it’s there to make sure that the properties in question are private. As you probably know, unlike other languages, JavaScript doesn’t natively support private properties, so there is a way to do that with WeakMap objects. (On top of that, I’m not sure why you also have them as variables that are not only not used, but also don’t have explicit const or let declarations.)

If we look at your current implementation of the _nTests, _nFailedTests and _nPassedTests properties, for example, anyone can go ahead and manually change them lie so: this._nTests = ‘some string’; and that will break the library. With a WeakMap and a getter there is no way to do that.

Looking at your assertEquals() function, why did you choose to create a new method instead of improving the current assert() (without breaking backwards compatibility of course)? On of my ideas for assert() was that it would be a universal method that could test anything, provided that the developer knows how to run their comparisons. I understand that for some developers having built-in comparisons is easier, but then we would probably need to go down the road of also including assertNotEquals(), assertTruthy(), assertLessThan(), assertContains(), assertMatches() and many others like other libraries like jest do. Is that something you were considering doing in the future? If so, let’s discuss.

On more minor improvements, one thing I try to stay away from is if else statements. It’s a matter of style and my personal preference, I believe it makes code cleaner and I would like to keep it that way in this code base. You can check out some thoughts in this article for instance.

Also, be careful when you use the == operator, in many cases you want to use === especially when unit testing. In your assertEquals() function, you have the following line:

let condition = expectedResult == result;

The problem is that when I run this test:

test.assertEquals(() => addTwoValues(5, 2), '7', '== vs ===');

it should return false (beause addTwoValues() return a Number where as ’7’ is a String), however it returns true, hence that == of yours should be ===.

Let me say again how much I appreciate your ideas and the time you invested, and I’m more than happy to hear back your thoughts and whether you are open to making some improvements before I merge.

@dlealv
Copy link
Author

dlealv commented Oct 2, 2021

Thanks, @WildH0g for taking the time to respond, I was working on some of the changes and I will update the source code, but before that, I have the following questions and comments:

  1. I was able to use WeakMap, it works. It makes it less friendly to increment counters, for example: _nTests.set(this, _nTests.get(this)+1); but it works.
  2. To avoid adding additional assert functions, I did it because I was not able to extend the existing assert without breaking backward compatibility. For example: assert(condition, message = null, expectedResult=true), forces a less friendly approach because when the third argument is provided and we test against the result of condition we don't really need the second argument message so I was not able to find a solution without breaking the compatibility. If you have some suggestions in this regard, please let me know.
  3. About if-else comments, I see you are using in your assert and if-else block but without {} because you need a single line, but I need more lines. Would you provide a specific example in my code of how it can be converted? I was reading the link you shared, but I don't really know which specific situation may apply.
  4. Reading the article about conditionals, I realized what we don't check for undefined for input arguments for assert and assertEquals. For example, in the case of assert() with no input argument, the output will be FAIL, when it was not even validated. It should throw an exception, right? (or an early return, but the user would know what happened), is this the expected test case result?, similarly for the case of: assert(1==1), with the second input argument undefined, it makes more sense because it returns: ✔ PASSED: undefined indicating the user didn't provide the message input argument. For assertEquals() with the current implementation the result is PASS and which is not the expected result, so I would need to correct this too. Similarly in the case of assertEquals (1==1) missing the second mandatory input argument. I would need to check the preconditions of the mandatory input arguments. Any thought about how to handle these situations? (my preference is to throw an Errorindicating the user the information that is missing). Based on the approach you prefer I would need to modify the source code.
  5. Fixed the `==``
  6. Reviewing my assertEquals it doesn't consider the situation where fun is a condition, so I need to modify it to consider this test case:
if ("function" === typeof fun) result = fun();
else result = fun;
  1. In your code for assert in case of error (catch), you are including the input argument message, I think it might confuse the user:
} catch(err) {
        console.log(`❌ FAILED: ${message} (${err})`);
}

I would recommend to log out just:

} catch(err) {
        console.log(`❌ FAILED(err): ${err}`);
}

There are some situations where it makes sense to include the message, but in the case of undefined values of the input argument, maybe not. It is a matter of preference on how the library is designed, and just to act for other cases or methods consistently.

Thanks so much for your help, I am really learning a lot with this library and your comments.

Please let me know, so I can proceed with the changes and also to updates the documentation I prepared.

David

let msg, result;

try {
if ("function" === typeof fun) {
Copy link
Owner

Choose a reason for hiding this comment

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

I know I do this in my code too, but we could user a ternary operator here you and I, it would take down one if :)

Copy link
Author

Choose a reason for hiding this comment

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

About this suggestion, In my last version it is different for assertEquals, for assert you have the following line:
if ("function" === typeof condition) condition = condition();

which is correct, we don't have an else condition, so there is no way to use the ternary operator. If I am not correct, please advise. Thanks

@WildH0g
Copy link
Owner

WildH0g commented Oct 4, 2021

Hey @dlealv,

I left some comments in your code. I din't see the WeakMap though, strange.

Regarding extending a function, there is no native function overloading in JavaScript, but it could be done by reading the arguments object. This article gives an idea of how it works, but I don't find their code super clean, we can do better :)

Most of my comments are related to style, you don't have any bugs, so I'm happy to move forward. I am open to proceeding in any of the two ways:

  • you can continue working on your code with my comments
  • I merge your code in a different branch, implement some changes from my side and then merge to master

Both ways work for me, I'll let you decide.

@dlealv
Copy link
Author

dlealv commented Oct 4, 2021

It looks you assumed I made the changes in the source code in GitHub, I did the changes I indicated in my comment on my local copy waiting for some of the comments/questions. Mainly related to avoiding adding an additional assert function, I see you shared an article, I will take a look at it. I prefer to work based on your comments. Today I am going to update the latest version and then we can move from there. I will in the meantime to evaluate how to avoid adding a new assert function..

dlealv added 2 commits October 4, 2021 12:54
Added the changes I mentioned in my last post. I included also how to treat undefined input argument for `assertEquals` only, waiting for your feedback
Updated the file, with additional tests, including when input argument is a condition (not a funcion) and when input argument are `undefined`
@dlealv
Copy link
Author

dlealv commented Oct 4, 2021

Hi, @WildH0g I updated the source code with the changes I mentioned and also the corresponding testing file. I included checking for preconditions against undefined for assertEquals just to show this approach throwing and Error, but not for assert, waiting for your feedback. I removed the if-curly brackets based on your last feedback and replaced if ("function" === typeof fun) using ternary operator too. PENDING, to review the option to extend your assert to include assertEquals functionality without breaking backward compatibility. I will review the article you shared with me. Thanks a lot for your help. It is also pending my suggestion about catch and to include message only when it is necessary. In assertEquals I am considering it now, waiting for your consideration.

@dlealv
Copy link
Author

dlealv commented Oct 4, 2021

About extending assert, I read the article. I see how we can do it, the thing is the order or argument and the fact that message and expectedResult can both be a string and in case the expectedResult is provided, there is no need for message input argument.

How we can differentiate the following situation:
assert ("Hi", "Wrong"); expected to be invoked assertEquals("Hi", "Wrong"), with the second argument not being the message, but instead the expected result. It could be interpreted as assert("Hi", "Wrong") where "Wrong" is message input argument. Both argument messageand expectedResult can be strings.

Thanks,

David

dlealv added 5 commits October 4, 2021 16:29
Removed unnecessary curly bracket under assert.
Simplified the catch block for assertEquals.
Removing curly bracket from printSubHeader, printSummary. Added documentation for resetTestCounters and printSummary,
Minor corrections.
Changed in printSummary the output "Some Tests Failed" to "Some Tests FAILED".
@dlealv
Copy link
Author

dlealv commented Oct 6, 2021

@WildH0g I have another question about the README file, you presented all the samples using this notation ()=> why is that, I realized that the library works not using that notation, is there any difference?

for example the following piece of code provides the same results:

  test.assertEquals(addTwoValues(1, 2), 3);
  test.assert(addTwoValues(1, 2) == 3, "Valid case: 1 + 2 = 3");
  test.assertEquals(addTwoValues(1,2),3, "Expected result: 1+2 = 3"); 
  test.assertEquals(addTwoValues(1,2),4, "Wrong result because 1+2 should be equal to 3");

we get the same result:

✔ PASSED
✔ PASSED: Valid case: 1 + 2 = 3
✔ PASSED: Expected result: 1+2 = 3
❌ FAILED: Wrong result because 1+2 should be equal to 3

but we don't provide such samples in the README file so people may think we need to use the library only with ()=> form.

Thank your clarification

David

try {
// Checking preconditions
if (fun === undefined) throw new Error("Provide 'fun' input argument");
if (expectedResult === undefined) throw new Error("Provide 'expectedResult' input argument");
Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, I'm now thinking what if we want a condition to be undefined, I mean that's not crazy, is it?

Copy link
Author

Choose a reason for hiding this comment

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

if you are OK, I will take into account for your assert in a similar way I did for assertEquals

Copy link
Owner

Choose a reason for hiding this comment

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

What I'm saying is that I don't think we should be throwing an error when our condition or expected result (or both) are undefined. We sometimes want them to be undefined.

Copy link
Author

Choose a reason for hiding this comment

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

oh ok, in such a case, then I can force PASS (because fun === expectedResult) for example to avoid throwing an error, then we need to document this test case too. Is it ok for you?

@WildH0g
Copy link
Owner

WildH0g commented Oct 11, 2021

How we can differentiate the following situation: ...

That's actually a valid point. Having given it some thought, I think we should not allow any tests without a message. I mean it's just bad practice. So two arguments would be a condition and a message, three arguments would be a condition, the expected result and a message. What do you think?

I have another question about the README file, you presented all the samples using this notation ()=> why is that

You're right they work both ways, but not passing a function is incompatible with catchErr(), so for consistency I think it's best to use callbacks everywhere.

And thanks for this update, it's starting to look good. I left a comment on the files, I will run a few more tests and come back to you ASAP with a couple suggestions

@dlealv
Copy link
Author

dlealv commented Oct 11, 2021

How we can differentiate the following situation: ...

That's actually a valid point. Having given it some thought, I think we should not allow any tests without a message. I mean it's just bad practice. So two arguments would be a condition and a message, three arguments would be a condition, the expected result and a message. What do you think?

The thing at least for assertEquals, I consider the message is not necessary in case the test passed, there is nothing to alert the user (the message: PASS is self-explanatory). That is why I consider message as optional. When the test fails then, a message is provided in the form: result != expectedResult (automatically created informing the user about the mismatch) I consider it enough for informing the user, given the option of providing an optional message in case the user provides it. Following your recommendation, we can consider a default message when it is passed in the form of: result == expectedResult . Forcing the user to provide a message on every case for assertEqual, is redundant, especially when a meaningful message can be built automatically (saving the user time). Please let me know and I can do the changes. That was the main reason I created assertEquals, for me it was not relevant the message from assert I wanted to see the mismatch, to correct the error during the next execution.

assert function is implicitly an assert true condition because it passes if the condition is true (line: if (condition) {..}). An assert equal is more generic in my opinion, because a true condition is just a particular case of an equal condition. For example, the design of catchErr(callback, errorMessage, message) is more similar to assertEquals: something to check, expected value, and message. I know you have in mind that assert would be the simplest way to define a unit test library, but it is less generic. Because it is an assert true condition at the end, the expected value is not required, but then it is difficult to extend to a more general case. For example assertEquals(fun, true, message) is equivalent to assert(fun, message) in terms of function declaration, but vice-versa is not possible. I know it is a more philosophical topic.

I have another question about the README file, you presented all the samples using this notation ()=> why is that

You're right they work both ways, but not passing a function is incompatible with catchErr(), so for consistency I think it's best to use callbacks everywhere.

OK, then we can include a note in the README file indicating that.

And thanks for this update, it's starting to look good. I left a comment on the files, I will run a few more tests and come back to you ASAP with a couple suggestions

OK, I will review them once you provide your comments. Thanks for your review.

Removed the condition for throwing an error in case no mandatory input arguments are provided.
Added more tests for undefined input values
Adding a comment about `=>` arrow function in `catchErr section.
In catchErr the local variable error is never used, removed the declaration and the line error = err, because then the variable error is not used.
Added specific tests for testing catchErr, considering both scenarios: 1) When the error message matches and 2) When the error message doesn't match
minor corrections
In is2dArray inverted the typeof checking as it is in other functions of the library.
Added a minor improvement for `catchErr` to consider the input argument `message`optional, because the message can be built automatically if the user doesn't provide it.
Added the corresponding tests for `catchErrType` and `catchErr`.
@dlealv
Copy link
Author

dlealv commented Oct 23, 2021

@WildH0g you seem pretty busy. I have some time and I added new functionality for testing not just the error message but also the error type. The name of the function is: catchErrType, you can see in the corresponding testing file how it works. It allows testing also Error type and the corresponding error message. I didn´t extend it from catchErrbecause we should change the order of the input argument, or maybe not because the input argument errorType can be differentiated from message since it is a string. I didn't update the README file, because I would like to know your feedback first. I considered also the input argument messageas optional because the message if it is not provided it be built automatically for the user. Please let me know your thoughts. Thanks. If I am able to unite them I will update it later.

Corrected my previous version. Now`catchErr` extending its signature using default input arguments, can also test error message and error type. Reviewed documentation. Added minor changes to `assert` to be used consistently for the message to print out in case the test passes with an empty message. We use this function for sending messages to `catchErr` so both functions behave the same under this scenario. For `assertEquals` now we wrap some message with quotes (') when it makes sense.
Added additional tests for testing the new changes.
@dlealv
Copy link
Author

dlealv commented Oct 24, 2021

@WildH0g I have time during the weekend to make the changes I mentioned: Corrected my previous version. NowcatchErr extending its signature using default input arguments, can also test error message and error type. Reviewed documentation. Added minor changes to assert to be used consistently for the message to print out in case the test passes with an empty message. We use this function for sending messages to catchErr so both functions behave the same under this scenario. For assertEquals now we wrap some message with quotes (') when it makes sense.

Waiting for your feedback, if you agreed, then I move on and update the READMEfile. Thanks

@WildH0g
Copy link
Owner

WildH0g commented Oct 26, 2021

Hey @dlealv , join me on Gitter for better communication on the topic ;)

Documented the new changes will be released today
Summary of the changes:
1. This version has consistent behaviour for the messages via (input argument `message`). When the user doesn't provide a message a default built-in message will be provided in both cases: PASS and FAIL
2. Improved documentation
3. Improved printSummary()
4. Considered Error label in case of Error and counted as a failure and using `console.error()` in instead of `console.log()`
5. Using setter and getter for attribute `levelInfo`
6. Added a safeguard for setting `levelInfo` to ensure numbers only. I considered throwing an error
7. Encapusalated the check if the test is not active in a private function: `stopIfNotActive_` in order to have a cleaner code and easier to maintain
Added the corresponding tests and verification for the version uploaded today.
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dlealv
Copy link
Author

dlealv commented Oct 31, 2021

Hi @WildH0g

Added the following changes in the version I uploaded today. I don't envision any other future changes from my end. IMHO now we have something more solid from my previous versions. I also considered some of your suggestions about providing always a message. At this point only waiting for some feedback from your end

Here is the summary of the changes:

  1. This version has consistent behavior for the messages via (input argument message). When the user doesn't provide a message a default built-in message will be provided in both cases: PASS and FAIL
  2. Improved documentation
  3. Improved printSummary()
  4. Considered Error label in case of error, it counts as a failure but using console.error() in instead of console.log()
  5. Using setter and getter for attribute levelInfo
  6. Added a safeguard for setting levelInfo to ensure numbers only. I considered throwing an error
  7. Encapusalated the check if the test is not active in a private function: stopIfNotActive_ in order to have a cleaner code and easier to maintain

Thanks!

David

@mark05e
Copy link

mark05e commented Mar 15, 2022

Hi @WildH0g just curious to see if there are any roadblocks with taking on these enhancements provided by dlealv

@mark05e
Copy link

mark05e commented Mar 17, 2022

Hey @dlealv I did a quick test locally with your mods, with runInGas(true) and I get the results below

levelInfo = 0
Reset counters
Testing assertEquals
********************
Expected: Some tests failed (one line)
Reset counters
Shows only the summary line, the counters are reseted so no tests, therefore all test passed
Testing assert
**************
Reset counters
Expected to see some test failed
Testing the case all test passed with silent mode
Reseting the counters
Testing with assert, under silent mode: one test executed and passed
Printing the summary line only: all test passed
Changing the level info to 1
Showing now printSummary with the summary line only from the previous set of tests
Changing the level info to 1

and when I run the test with runInGas(false), I get

*****************************************
* Testing addTwoValues using assertEquals
*****************************************
** Using default Level Info value (1) for 'addTwoValues' function
** Expected: Test 1 pass user message, Test 2 pass with default, Test 3 fail user message, Test 4 fails with default message
✔ PASSED: Valid case: 2 + 2 = 4
✔ PASSED: 3 === 3
❌ FAILED: Expected to fail, because 1 + 2 != 4
❌ FAILED: 3 != 4
** Expected: 4-Test, 2-Tests fail, 2-Tests Pass
C:\xx\tests\UnitTestingApp.js:264
      if (this.levelInfo >= 1) console.log(Utilities.formatString(msg, _nTests.get(this),
                                           ^

ReferenceError: Utilities is not defined
    at UnitTestingApp.printSummary (C:\xx\tests\UnitTestingApp.js:264:44)
    at runTests (C:\xx\tests\TestTemplate.js:50:8)
    at C:\xx\tests\TestTemplate.js:482:20
    at Object.<anonymous> (C:\xx\tests\TestTemplate.js:483:3)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

I think the printSummary function is not setup to run locally - because of Utilities.formatString?

    printSummary() {
      if (this.stopIfNotActive_()) return;
      let msg = "TOTAL TESTS=%d, ❌ FAILED=%d, ✔ PASSED=%d";
      if (this.levelInfo >= 1) console.log(Utilities.formatString(msg, _nTests.get(this),
        _nFailTests.get(this), _nPassTests.get(this)));
      console.log((_nFailTests.get(this) == 0) ? "ALL TESTS ✔ PASSED" : "❌ Some Tests FAILED");
    }

I am still reviewing the messages while running it on GAS but thought I'd let you know what I found so far.

@dlealv
Copy link
Author

dlealv commented Mar 17, 2022

Hi @WildH0g just curious to see if there are any roadblocks with taking on these enhancements provided by dlealv

Hey @dlealv I did a quick test locally with your mods, with runInGas(true) and I get the results below

levelInfo = 0
Reset counters
Testing assertEquals
********************
Expected: Some tests failed (one line)
Reset counters
Shows only the summary line, the counters are reseted so no tests, therefore all test passed
Testing assert
**************
Reset counters
Expected to see some test failed
Testing the case all test passed with silent mode
Reseting the counters
Testing with assert, under silent mode: one test executed and passed
Printing the summary line only: all test passed
Changing the level info to 1
Showing now printSummary with the summary line only from the previous set of tests
Changing the level info to 1

and when I run the test with runInGas(false), I get

*****************************************
* Testing addTwoValues using assertEquals
*****************************************
** Using default Level Info value (1) for 'addTwoValues' function
** Expected: Test 1 pass user message, Test 2 pass with default, Test 3 fail user message, Test 4 fails with default message
✔ PASSED: Valid case: 2 + 2 = 4
✔ PASSED: 3 === 3
❌ FAILED: Expected to fail, because 1 + 2 != 4
❌ FAILED: 3 != 4
** Expected: 4-Test, 2-Tests fail, 2-Tests Pass
C:\xx\tests\UnitTestingApp.js:264
      if (this.levelInfo >= 1) console.log(Utilities.formatString(msg, _nTests.get(this),
                                           ^

ReferenceError: Utilities is not defined
    at UnitTestingApp.printSummary (C:\xx\tests\UnitTestingApp.js:264:44)
    at runTests (C:\xx\tests\TestTemplate.js:50:8)
    at C:\xx\tests\TestTemplate.js:482:20
    at Object.<anonymous> (C:\xx\tests\TestTemplate.js:483:3)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

I think the printSummary function is not setup to run locally - because of Utilities.formatString?

    printSummary() {
      if (this.stopIfNotActive_()) return;
      let msg = "TOTAL TESTS=%d, ❌ FAILED=%d, ✔ PASSED=%d";
      if (this.levelInfo >= 1) console.log(Utilities.formatString(msg, _nTests.get(this),
        _nFailTests.get(this), _nPassTests.get(this)));
      console.log((_nFailTests.get(this) == 0) ? "ALL TESTS ✔ PASSED" : "❌ Some Tests FAILED");
    }

I am still reviewing the messages while running it on GAS but thought I'd let you know what I found so far.

Thank

Hey @dlealv I did a quick test locally with your mods, with runInGas(true) and I get the results below

levelInfo = 0
Reset counters
Testing assertEquals
********************
Expected: Some tests failed (one line)
Reset counters
Shows only the summary line, the counters are reseted so no tests, therefore all test passed
Testing assert
**************
Reset counters
Expected to see some test failed
Testing the case all test passed with silent mode
Reseting the counters
Testing with assert, under silent mode: one test executed and passed
Printing the summary line only: all test passed
Changing the level info to 1
Showing now printSummary with the summary line only from the previous set of tests
Changing the level info to 1

and when I run the test with runInGas(false), I get

*****************************************
* Testing addTwoValues using assertEquals
*****************************************
** Using default Level Info value (1) for 'addTwoValues' function
** Expected: Test 1 pass user message, Test 2 pass with default, Test 3 fail user message, Test 4 fails with default message
✔ PASSED: Valid case: 2 + 2 = 4
✔ PASSED: 3 === 3
❌ FAILED: Expected to fail, because 1 + 2 != 4
❌ FAILED: 3 != 4
** Expected: 4-Test, 2-Tests fail, 2-Tests Pass
C:\xx\tests\UnitTestingApp.js:264
      if (this.levelInfo >= 1) console.log(Utilities.formatString(msg, _nTests.get(this),
                                           ^

ReferenceError: Utilities is not defined
    at UnitTestingApp.printSummary (C:\xx\tests\UnitTestingApp.js:264:44)
    at runTests (C:\xx\tests\TestTemplate.js:50:8)
    at C:\xx\tests\TestTemplate.js:482:20
    at Object.<anonymous> (C:\xx\tests\TestTemplate.js:483:3)
    at Module._compile (node:internal/modules/cjs/loader:1103:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1155:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Function.Module._load (node:internal/modules/cjs/loader:822:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

I think the printSummary function is not setup to run locally - because of Utilities.formatString?

    printSummary() {
      if (this.stopIfNotActive_()) return;
      let msg = "TOTAL TESTS=%d, ❌ FAILED=%d, ✔ PASSED=%d";
      if (this.levelInfo >= 1) console.log(Utilities.formatString(msg, _nTests.get(this),
        _nFailTests.get(this), _nPassTests.get(this)));
      console.log((_nFailTests.get(this) == 0) ? "ALL TESTS ✔ PASSED" : "❌ Some Tests FAILED");
    }

I am still reviewing the messages while running it on GAS but thought I'd let you know what I found so far.

Hi @mark05e thanks for taking the time to review it. I was trying to review the issue, but I am not able to reproduce your error. I did the following: On script: TestForVersion_0.1.1.js

I changed the following function:

function runTests() {
  const test = new UnitTestingApp();
  test.enable();
  test.clearConsole();
  test.runInGas(false); // changes from true to false
...
}

then execute the function runTests() and I didn't get any errors.

Please provide more details about the testing scenario you ran.

Thanks

@mark05e
Copy link

mark05e commented Mar 17, 2022

@dlealv I have redownload the scripts into my test directory

and modified runInGas(false) and also added runTests() at the end of the file.

...
    test.printHeader("Testing enable = false, the print-family functions print no information");
    test.disable();
    test.printHeader("No expected output");
    test.printSubHeader("No expected output");
    test.printSummary();
  }
  runTests();
//End of file

Then I run the tests locally using node TestForVersion_0.1.1.js. I still get the same error. I am using node version 16.14.0

@dlealv
Copy link
Author

dlealv commented Mar 17, 2022

@dlealv I have redownload the scripts into my test directory

and modified runInGas(false) and also added runTests() at the end of the file.

...
    test.printHeader("Testing enable = false, the print-family functions print no information");
    test.disable();
    test.printHeader("No expected output");
    test.printSubHeader("No expected output");
    test.printSummary();
  }
  runTests();
//End of file

Then I run the tests locally using node TestForVersion_0.1.1.js. I still get the same error. I am using node version 16.14.0

Thanks for your reply @mark05e I don't use node, so I never tested in node. All my tests are using Google Apps Script. Maybe as you said there may be some incompatibility with Utilities. formatting we can change that, have you tried to replace this utility function? I am pretty sure I am using this functionality in other places in the script.

@mark05e
Copy link

mark05e commented Mar 18, 2022

@dlealv Utilities.formatString is from GAS and does not exist in the browser or in node. I will debug to see which variable stores the flag to see if we are running in GAS and then prepare an alternative statement for those console statements. I'm relatively new to coding so it might take a bit.

@mark05e
Copy link

mark05e commented Mar 19, 2022

@dlealv here is my updated codeblock for this function. I tested this out locally and on GAS and it seems to work. I just removed Utilities.formatString usage. It does not seem to be used anywhere else in the code.

      printSummary() {
        if (this.stopIfNotActive_()) return;
        let msg = "TOTAL TESTS=%d, ❌ FAILED=%d, ✔ PASSED=%d";
        if (this.levelInfo >=1) console.log(msg, _nTests.get(this), _nFailTests.get(this), _nPassTests.get(this));
        console.log((_nFailTests.get(this) == 0) ? "ALL TESTS ✔ PASSED" : "❌ Some Tests FAILED");
      }

@WildH0g
Copy link
Owner

WildH0g commented Mar 20, 2022

@dlealv @mark05e Thanks both for your inteterest and @dlealv sorry for my delay. Answering the question if there are any roadbocks, not major ones, just limited time. I think I can have this sorted out next weekend if that's OK with both of you.

@mark05e
Copy link

mark05e commented Mar 20, 2022

All good @WildH0g. I started doing some work ontop of @dlealv 's work - cleanup on the Readme page and made the changes related to the comments that were mentioned earlier. Currently I have set it up on my fork of the project https://github.com/mark05e/UnitTestingAppForGAS

@dlealv
Copy link
Author

dlealv commented Mar 22, 2022

@dlealv here is my updated codeblock for this function. I tested this out locally and on GAS and it seems to work. I just removed Utilities.formatString usage. It does not seem to be used anywhere else in the code.

      printSummary() {
        if (this.stopIfNotActive_()) return;
        let msg = "TOTAL TESTS=%d, ❌ FAILED=%d, ✔ PASSED=%d";
        if (this.levelInfo >=1) console.log(msg, _nTests.get(this), _nFailTests.get(this), _nPassTests.get(this));
        console.log((_nFailTests.get(this) == 0) ? "ALL TESTS ✔ PASSED" : "❌ Some Tests FAILED");
      }

Thanks, @mark05e it looks good to me, I didn't remember we can invoke console.log using substitution strings.

I took a look at your fork and I liked the links you added to the readme page. Thanks!

All good @WildH0g. I started doing some work ontop of @dlealv 's work - cleanup on the Readme page and made the changes related to the comments that were mentioned earlier. Currently I have set it up on my fork of the project https://github.com/mark05e/UnitTestingAppForGAS

@dlealv
Copy link
Author

dlealv commented Mar 22, 2022

@dlealv @mark05e Thanks both for your inteterest and @dlealv sorry for my delay. Answering the question if there are any roadbocks, not major ones, just limited time. I think I can have this sorted out next weekend if that's OK with both of you.

@WildH0g don't worry, when you get a chance, hopefully, we can have those changes finally promoted.

@dlealv
Copy link
Author

dlealv commented Sep 29, 2022

It has been a long time since we were working on such improvements, is there any update from you? @WildH0g @mark05e I would like to see the changes live. Thanks.

@mark05e
Copy link

mark05e commented Oct 14, 2022

Hi @dlealv - I think WildH0g might be busy. I would suggest you create your own fork of this project and maintain your changes on the fork. Also drop in a link to the project here so others can find it.

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.

3 participants