Y.StringAssert #1474

Open
wants to merge 5 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

customcommander commented Dec 8, 2013

Hey guys,

I use YUI Test a lot at work and this PR is based on ideas I had when writing string assertions.

Even though string assertions are not the majority of the tests I write, they would have been a bit easier to write if I had the appropriate methods. But also the failure messages would have made more sense.

Examples:

// before
"should start with foo": function () {
    var str = "foobar";
    Y.Assert.isTrue(str.indexOf("foo")==0);
}

// after
"should start with foo": function () {
    Y.StringAssert.startsWith("foo", "foobar");
}

// before
"should only contains number": function () {
    var re = /^[0-9]+$/
    Y.Assert.isTrue(re.test("123"));
}

// after
"should only contains number": function () {
    Y.StringAssert.matches(/^[0-9]+$/, "123");
}

This PR provides a new StringAssert class to the Test module with the following methods:

  • isEmpty()
  • isNotEmpty()
  • isTrimmed()
  • contains()
  • doesNotContain()
  • matches()
  • doesNotMatch()
  • startsWith()
  • doesNotStartWith()
  • endsWith()
  • doesNotEndWith()

Also included in this PR:

  • Unit tests for each method
  • Update of the build configuration
  • Update of the HISTORY
  • User guide documentation

Thanks

@rgrove rgrove commented on the diff Dec 9, 2013

build/test/test-debug.js
@@ -3021,6 +3021,379 @@ YUITest.DateAssert = {
}
};
+
+/**
+ * Provides functions to test JavaScript strings for a variety of cases.
+ *
+ * @namespace Test
@rgrove

rgrove Dec 9, 2013

Contributor

Minor nitpick, but I recommend avoiding the @namespace tag and just specifying the full namespace in the @class tag:

@class Test.StringAssert

It's clearer to read and will only set the namespace for this class, whereas @namespace will affect everything until the next @namespace tag is reached, which can be confusing.

@rgrove rgrove commented on the diff Dec 9, 2013

build/test/test-debug.js
+ * If `excl_whitespace` is set to true the string will be trimmed first, which will
+ * pass the test if the string only contains whitespaces.
+ *
+ * @example
+ * Y.StringAssert.isEmpty(""); // pass
+ * Y.StringAssert.isEmpty(" \r\n \t ", true); // pass
+ * Y.StringAssert.isEmpty("hello world"); // fail
+ * Y.StringAssert.isEmpty(" \r\n \t "); // fail
+ *
+ * @param {String} str The string to test.
+ * @param {Boolean} [excl_whitespace=false] If true the string will be trimmed first.
+ * @param {String} [message] The message to display if the assertion fails.
+ * @method isEmpty
+ * @static
+ */
+ isEmpty: function (str, excl_whitespace, message) {
@rgrove

rgrove Dec 9, 2013

Contributor

I think isEmpty() would be better off not optionally trimming whitespace, since that can easily be done by the caller.

The intent of a test like:

Y.Test.StringAssert.isEmpty(Y.Lang.trim(foo));

...is more clear at a glance than:

Y.Test.StringAssert.isEmpty(foo, true);

...which is a boolean trap.

We don't need to worry about the behavior of Y.Lang.trim() on non-string values, since it will return them unmodified.

I also can't think of an existing assertion method whose behavior changes based on an optional parameter, and I think it's a good idea to maintain that consistency.

@customcommander

customcommander Dec 9, 2013

Contributor

A boolean trap is always obvious once it's pointed out and your last point makes sense too. So I've got no other choice than making the change. Thanks :-)

Done.

@rgrove rgrove commented on the diff Dec 9, 2013

build/test/test-debug.js
+ * If `excl_whitespace` is set to true the string will be trimmed first, which will
+ * fail the test if the string only contains whitespaces.
+ *
+ * @example
+ * Y.StringAssert.isNotEmpty("hello world"); // pass
+ * Y.StringAssert.isNotEmpty(" \r\n \t "); // pass
+ * Y.StringAssert.isNotEmpty(""); // fail
+ * Y.StringAssert.isNotEmpty(" \r\n \t ", true); // fail
+ *
+ * @param {String} str The string to test.
+ * @param {Boolean} [excl_whitespace=false] If true the string will be trimmed first.
+ * @param {String} [message] The message to display if the assertion fails.
+ * @method isNotEmpty
+ * @static
+ */
+ isNotEmpty: function (str, excl_whitespace, message) {
@rgrove

rgrove Dec 9, 2013

Contributor

Same feedback as isEmpty() above. I think we should remove the trimming option.

@rgrove rgrove commented on the diff Dec 9, 2013

build/test/test-debug.js
+
+ Y.Assert._increment();
+
+ excl_whitespace = (Y.Lang.isBoolean(excl_whitespace) && excl_whitespace);
+
+ if (!Y.Lang.isString(str)) {
+ throw new TypeError("StringAssert.isEmpty(): str is not a string");
+ }
+
+ if (excl_whitespace) {
+ str = Y.Lang.trim(str);
+ }
+
+ if (str.length) {
+ message = Y.Assert._formatMessage(message, "String is not empty");
+ Y.fail(message);
@rgrove

rgrove Dec 9, 2013

Contributor

Shouldn't this be Y.Test.Assert.fail()?

@rgrove rgrove commented on the diff Dec 9, 2013

build/test/test-debug.js
+
+ Y.Assert._increment();
+
+ excl_whitespace = (Y.Lang.isBoolean(excl_whitespace) && excl_whitespace);
+
+ if (!Y.Lang.isString(str)) {
+ throw new TypeError("StringAssert.isNotEmpty(): str is not a string");
+ }
+
+ if (excl_whitespace) {
+ str = Y.Lang.trim(str);
+ }
+
+ if (!str.length) {
+ message = Y.Assert._formatMessage(message, "String is empty");
+ Y.fail(message);
@rgrove

rgrove Dec 9, 2013

Contributor

Shouldn't this be Y.Test.Assert.fail()?

@rgrove rgrove commented on the diff Dec 9, 2013

build/test/test-debug.js
+ * @param {String} str The string to test.
+ * @param {String} [message] The message to display if the assertion fails.
+ * @method isTrimmed
+ * @static
+ */
+ isTrimmed: function (str, message) {
+
+ Y.Assert._increment();
+
+ if (!Y.Lang.isString(str)) {
+ throw new TypeError("StringAssert.isTrimmed(): str is not a string");
+ }
+
+ if (Y.Lang.trim(str).length != str.length) {
+ message = Y.Assert._formatMessage(message, "String is not trimmed");
+ Y.fail(message);
@rgrove

rgrove Dec 9, 2013

Contributor

Ditto above re. Y.Test.Assert.fail(). I'll stop commenting on this line in every assertion, but I feel like they all need to change, unless there's something I'm missing.

@customcommander

customcommander Dec 9, 2013

Contributor

I am happy to make that change but since Y.fail is a documented shortcut (See http://yuilibrary.com/yui/docs/api/classes/YUI.html#method_fail) I felt it was OK to use it.

What do you think?

@rgrove

rgrove Dec 9, 2013

Contributor

Ah, good point! I was mistakenly thinking of Y.error(), which is totally different. Y.fail() should be fine.

Contributor

rgrove commented Dec 9, 2013

Thanks for this, @customcommander! I'm in favor of merging this in after the comments above have been addressed and after other folks have weighed in.

Sorry for commenting on the built file like a dope; in the future, feel free to leave built files out of pull requests (we'll build after merging).

I'm not entirely sure where YUI Test is being maintained these days; I know there's an external project, but I don't know if that's meant to be the source of truth or if the version in YUI itself is the source of truth. Does anyone else know?

Contributor

customcommander commented Dec 9, 2013

Thanks for the feedback @rgrove. Also just realised that I forgot to remove the build commit, sorry about that.

I've made all the changes that you asked except for Y.fail as I made a comment about it.

Contributor

rgrove commented Dec 13, 2013

@ericf, @triptych, or anyone else: How is YUI Test being maintained these days? Should this PR be merged into the YUI repo, or does it need to be moved to the standalone YUI Test repo?

Contributor

customcommander commented Feb 13, 2014

@ericf is there any update on this question? Should I move this to the standalone YUI Test?

Member

juandopazo commented Feb 13, 2014

It should be merged into YUITest, not YUI.

Contributor

customcommander commented Feb 13, 2014

I'm a bit confused now as to why #1308 got merged in YUI then.

I know that I mentioned YUI Test in my PR but actually what I meant is the test module in YUI.

I don't use the standalone project to write my tests as I need other stuffs in YUI. So it made sense to me to contribute to YUI. If this won't be available in YUI, I won't bother to migrate that code, sorry.

Feel free to close this PR.

Owner

caridy commented Feb 13, 2014

src/test is not in sync with yui/yuitest repo, which means that we should probably merge this one here, and eventually find a warrior willing to get them in sync, releasing a new version of YUITest, and replacing the guts in YUI to start using the files from yui/yuitest repo directly.

keep in mind that we already have a grunt file definition (https://github.com/yui/yui3/blob/master/src/test/Gruntfile.js) since yui/src/test is the source of truth, and we want to change that, where https://github.com/yui/yuitest becomes the source of truth and yui just pull the pieces from it to maintain backward compatibility mostly.

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