Submitting here.. #3

Merged
merged 3 commits into from Apr 13, 2012

Conversation

Projects
None yet
2 participants
@davglass

Submitting this request to the Almighty Zakas :)

@nzakas

This comment has been minimized.

Show comment Hide comment
@nzakas

nzakas Feb 22, 2012

The run() method accepts an options object that defines how the tests should be run. I think this would be better as an option on that object.

nzakas commented on 24fecdd Feb 22, 2012

The run() method accepts an options object that defines how the tests should be run. I think this would be better as an option on that object.

This comment has been minimized.

Show comment Hide comment
@davglass

davglass Feb 22, 2012

Owner

I didn't add it there because I wanted all tests to abide by this out of the box. With our large system, I didn't want to have to modify every single test module to add this parameter for backward compatibility. This gives a "one stop" place to set this and have it "just work" as it did before that assert was added.

Owner

davglass replied Feb 22, 2012

I didn't add it there because I wanted all tests to abide by this out of the box. With our large system, I didn't want to have to modify every single test module to add this parameter for backward compatibility. This gives a "one stop" place to set this and have it "just work" as it did before that assert was added.

This comment has been minimized.

Show comment Hide comment
@nzakas

nzakas Feb 22, 2012

I understand the intent, the problem is that this would create two places to specify options instead of one. I don't like the idea of having a global configuration for the TestRunner because your tests may have different results depending on the TestRunner being used...that should never happen. Also, having tests without asserts is a horrible practice, and I don't want to encourage that.

That being said, I'm willing to compromise with you. If you want this property, mark it as private and deprecated, and prefix it with an underscore. That way, at least it will be hidden or flagged in docs as something people shouldn't use. It can be a YUI-only thing.

I understand the intent, the problem is that this would create two places to specify options instead of one. I don't like the idea of having a global configuration for the TestRunner because your tests may have different results depending on the TestRunner being used...that should never happen. Also, having tests without asserts is a horrible practice, and I don't want to encourage that.

That being said, I'm willing to compromise with you. If you want this property, mark it as private and deprecated, and prefix it with an underscore. That way, at least it will be hidden or flagged in docs as something people shouldn't use. It can be a YUI-only thing.

This comment has been minimized.

Show comment Hide comment
@davglass

davglass Feb 22, 2012

Owner

That works! On it..

Owner

davglass replied Feb 22, 2012

That works! On it..

davglass added some commits Feb 22, 2012

Added version check to "flush" work around
    In 0.6.x the drain event does not fire, which means
    the process.exit(code) is never executed.
    This prohibits automated scripts from failing on a failed
    test.

nzakas added a commit that referenced this pull request Apr 13, 2012

@nzakas nzakas merged commit b722daf into nzakas:master Apr 13, 2012

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