-
Notifications
You must be signed in to change notification settings - Fork 8
Improvements and minor changes #1
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
base: master
Are you sure you want to change the base?
Conversation
@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.
|
Thanks very much, I will have a look shortly. Cheers. |
Updated the REAMDE file, based on the minor modification added.
|
WildHog on my fork I updated the |
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.
|
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 If we look at your current implementation of the Looking at your On more minor improvements, one thing I try to stay away from is Also, be careful when you use the let condition = expectedResult == result;The problem is that when I run this test: test.assertEquals(() => addTwoValues(5, 2), '7', '== vs ===');it should return 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. |
|
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:
I would recommend to log out just: There are some situations where it makes sense to include the 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 |
UnitTestingApp.js
Outdated
| let msg, result; | ||
|
|
||
| try { | ||
| if ("function" === typeof fun) { |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
|
Hey @dlealv, I left some comments in your code. I din't see the Regarding extending a function, there is no native function overloading in JavaScript, but it could be done by reading the 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:
Both ways work for me, I'll let you decide. |
|
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.. |
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`
|
Hi, @WildH0g I updated the source code with the changes I mentioned and also the corresponding testing file. I included checking for preconditions against |
|
About extending How we can differentiate the following situation: Thanks, David |
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".
|
@WildH0g I have another question about the README file, you presented all the samples using this notation 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: but we don't provide such samples in the README file so people may think we need to use the library only with Thank your clarification David |
UnitTestingApp.js
Outdated
| try { | ||
| // Checking preconditions | ||
| if (fun === undefined) throw new Error("Provide 'fun' input argument"); | ||
| if (expectedResult === undefined) throw new Error("Provide 'expectedResult' input argument"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
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?
You're right they work both ways, but not passing a function is incompatible with 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 |
The thing at least for
OK, then we can include a note in the
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`.
|
@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: |
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.
|
@WildH0g I have time during the weekend to make the changes I mentioned: Corrected my previous version. Now Waiting for your feedback, if you agreed, then I move on and update the |
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.
|
Kudos, SonarCloud Quality Gate passed! |
|
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:
Thanks! David |
|
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 and when I run the test with I think the 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
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: 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 Please provide more details about the testing scenario you ran. Thanks |
|
@dlealv I have redownload the scripts into my test directory and modified ...
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 fileThen I run the tests locally using |
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 |
|
@dlealv |
|
@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 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");
} |
|
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 |
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!
|
@WildH0g don't worry, when you get a chance, hopefully, we can have those changes finally promoted. |
|
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. |








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:
_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.printSummary()(for printing testing statistics) andprintSubHeader()for adding second level of header info,printHeader()Is more intended for the main testing title, and this one for specific sections.resetTestCounters()for resetting the statistical counters.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 optionalmessageargument in case the user wants to specify a different output message.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
WeakMapSo 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